Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option for issue-126: Convert str to None for location.td in well.plot() #136

Closed
wants to merge 2 commits into from
Closed

Option for issue-126: Convert str to None for location.td in well.plot() #136

wants to merge 2 commits into from

Conversation

dcslagel
Copy link
Contributor

@dcslagel dcslagel commented Jul 31, 2020

This is an alternative solution to pull-request #134 (issue #126 Resolve this issue at the most specific place it occurs in well.py):

Resolve this issue at the most specific place it occurs in well.py::plot():

        elif extents == 'td':
            try:
                # example code
                if isinstance(self.location.td, str):
                    self.location.td = None
                # end example code
                upper, lower = 0, self.location.td
            except:

The benefits of this solution are that it is seems simple and it is right at the location of the issue.

The drawbacks are that it clutters the processing code with data formatting code and that it might (but unlikely to) impact other code using self.location.td in other parts of the code. Initially I tried to stay away from this solution because applying this approach to many problems can fill an application with many local special cases rather than having a consistent way and central place for special case configuration. ... All that said, it is so simple to implement, and if this is a rare special case, it could be a good solution.

Changes:

  • Add a self.location.td check for invalid string values in well.plot().
    The value of self.location.td is used to set the 'lower' variable which
    must be either None or a number.
  • Include tests for this change in tests/test_location.py

Let me know if this change could be accepted (or rejected) or
needs some additional changes before being approved and merged.

Thank you,
DC

- Add a self.location.td check for invalid string values in well.plot().
The value of self.location.td is used to set the 'lower' variable which
must be either None or a number.
- Include tests for this change in tests/test_location.py
@dcslagel dcslagel changed the title Convert str to None for location.td in well.plot() Alt for issue-126 Convert str to None for location.td in well.plot() Jul 31, 2020
@dcslagel dcslagel changed the title Alt for issue-126 Convert str to None for location.td in well.plot() Option for issue-126 Convert str to None for location.td in well.plot() Jul 31, 2020
@dcslagel dcslagel changed the title Option for issue-126 Convert str to None for location.td in well.plot() Option for issue-126: Convert str to None for location.td in well.plot() Jul 31, 2020
@kwinkunks
Copy link
Member

Again, accepting the other PR and trying to fix the root cause. Thank you for the options!

@kwinkunks kwinkunks closed this Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants