Fix for issue 126: Add 'TD' str_to_none to location funcs

[dcslagel]

This pull request addresses issue 126 "TypeError 0 is not a string (plotting well imported from welly-generated LAS)"

It handles the cases where 'TD' (Total Depth) is set to a non-numeric string either because it wasn't in the LAS file or 'TD' was in the las file but with a character value. In these cases it sets well.location.td to the python NoneType object.

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

Thank you,
DC

[dcslagel]

Added test coverage for the change.

[kwinkunks]

Thank you very much for looking at this!

There's a bit of risk here because if a user tries to pass in a function to transform TD, it will be overwritten. (I see there's the same pattern for location; I guess I did that but I can't remember why.)

Maybe it would be smarter not to touch the function dictionary, but instead to add another one that is always used in the same way... Or to check for the issue explicitly.

What do you think? I'd value your opinion.

[dcslagel]

Yes, That is a good point.

See if this is a good way to go or can be improved?

For this pull-request (iteration), the user should be allowed to override the local funcs['TD'].
This would be a temporary solution that treats the local funcs['TD'] as a default setting that can be overridden.

• A message should be printed to the screen that this is happening
• 'location' could be handled the same way so there behavior is the same.
• If the user's func doesn't include 'TD' or 'location' then the local field func will run.

Your idea about a separate function dictionary suggests a more resilient design solution. This would be a significant change and should be separate issue/pull-request. There is a set of fields/mnemonics like 'TD', that Welly uses. It would be good to have routing tables for field specific code both for non-overridable code (run before funcs) and default code that can be replaced by `funcs[''].

Making sure 'TD' is set to None or a number is something that should happen every time regardless of a passed in funcs['TD'], so its str_to_none() function would be non-overridable function.

This routing table could be added to either defaults.py or to fields.py. It looks like fields.py is the natural location for it. Then location.py::from_lasio() could be changed to process any field specific non-over-ritable functions before calling the user's funcs and calling the field default functions only if the user's funcs don't override the field.

[dcslagel]

commit 710bf15 has these changes.

For this pull-request (iteration), the user should be allowed to override the local funcs['TD'].
This would be a temporary solution that treats the local funcs['TD'] as a default setting that can be overridden.

A message should be printed to the screen that this is happening
'location' could be handled the same way so there behavior is the same.
If the user's func doesn't include 'TD' or 'location' then the local field func will run.

from our earlier comments so you could see and evaluate whether this is a good way to go.

[dcslagel]

Continuing the discussion on alternative solutions for this, here a couple more ideas. I've implemented these and posted them for consideration as a possible solution.

1. On reading from_las() and from_lasio() if a header field/mnemonic is an empty string convert it to python's None type.

This could be good because it would be a general rule applied to all fields/mnemonic and any changes in subsequent code to handle this would be consistent across Welly. There may be other impacts that are less favorable if processing of other fields/mnemonics expect a str object. A draft implementation could be interesting.

2. Resolve this issue at the most specific place it occurs in well.py:

        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, I'll make a draft branch of it and post it so you have the option.

[dcslagel]

Just checking in on this and the related alternative solutions in #136 and #137.

Of these 3 approaches, it may be good to close the pull-requests for the options that aren't right for Welly.

If there is an option that look like a good direction but needs some changes let me know and I can work on it further.

Thanks,

DC

[kwinkunks]

As you suggest, I'd rather fix the root cause. Cheers!