Typo fix to connect ROMS reader to save interpolator

[kthyng]

I had the interpolator save and reuse working but did not realize it was not using the path I entered through the ROMS reader. I realized now that I called the interpolator filename variable two different things! I have fixed this now and also modified a few logger messages to be clearer.

[gauteh]

Thanks. I didn't catch it in the previous review, but I generally try to avoid the use of hasattr. It is better that this option is part of the class, and that the None value is used as not-set option. The hasattr pretty much introduces another order of complexity: value -> action vs variable exists -> value -> action, and it the introduced state (whether the variable is defined or not) is a frequent source of error (easy to forget if the var exists or not).

[kthyng]

@gauteh I wasn't aware of it being better not to use hasattr — thanks for telling me about this. What you say makes sense. It looks straight-forward to remove these in the ROMS reader, but in structured.py they seem necessary unless it is ok for me to add these as None somewhere initially?

[gauteh]

Yep 👍 it's better to add them as Nones, you effectively treat them as that anyway.

[kthyng]

@gauteh But where would I add "save_interpolator" and "interpolator_filename" as attributes to the class as None? I was trying to minimize my impact on the codebase so I didn't expand these attributes into all of the readers.

[gauteh]

You can add them here: https://github.com/OpenDrift/opendrift/blob/master/opendrift/readers/basereader/structured.py#L35

I think that is a good motivation, but it is a better tradeoff to add them as always existing variables (but not always used). As long as the variable is used in that class it belongs in that class or one of its parents.

[gauteh]

You can add them here: https://github.com/OpenDrift/opendrift/blob/master/opendrift/readers/basereader/structured.py#L35

Probably not an issue here: but you can only set them to None or any other immutable type here. If set to e.g. a string then modifying that string will propagate across all instances.

[kthyng]

@gauteh Is that reflected in the changes I made?

[gauteh]

Yes. I think this looks good! Thanks for fixing. Will wait for tests to finish, then merge.