search

NOAA-OWP / DMOD

Distributed Model on Demand infrastructure for OWP's Model as a Service
Other
7 stars 15 forks source link

Unreachable code in core DataDomain to_dict method #245

Open aaraney opened 1 year ago

aaraney commented 1 year ago

The following check can never be None.

https://github.com/NOAA-OWP/DMOD/blob/664d7571bbe578df272fbf916c25557ad558fe77/python/lib/core/dmod/core/meta_data.py#L699

It can never be None because of the following in DataFormat's initializer.

https://github.com/NOAA-OWP/DMOD/blob/664d7571bbe578df272fbf916c25557ad558fe77/python/lib/core/dmod/core/meta_data.py#L144-L150

I think the check in DataDomain's to_dict method should instead be:

  if not self.data_format.data_fields:

@robertbartel, can you confirm this?

robertbartel commented 1 year ago

Hmm, I believe you are correct. Pretty sure this was a missed component when the design of DataFormat changed, probably around the time StandardDatasetIndex was introduced.

Going a little further, it looks like the _custom_data_fields attribute itself probably needs to go. It's present in a lot of places but doesn't ever actually get a value set by __init__ (except when that's called by deserialization, which should make getting to this a catch-22).

aaraney commented 1 year ago

Thanks for having a look.

Going a little further, it looks like the _custom_data_fields attribute itself probably needs to go. It's present in a lot of places but doesn't ever actually get a value set by __init__ (except when that's called by deserialization, which should make getting to this a catch-22).

Right, so it looks like the only methods that will use _custom_data_fields are __eq__ and __hash__. I presume it's safe to remove _custom_data_fields from ContinuousRestriction? If that is the case, I can address this in #228.

robertbartel commented 1 year ago

I presume it's safe to remove _custom_data_fields from ContinuousRestriction?

Essentially yes, but from DataDomain.

If that is the case, I can address this in #228.

I hesitate slightly because of both scope creep and the potential for #228 to take a while to actually be finished and incorporated. For now, sure, go ahead with that. If #228 remains unresolved for more than another week, we should probably come back and separate this.

aaraney commented 1 year ago

Essentially yes, but from DataDomain.

Right, duh, I don't know why I wrote ContinuousRestriction.

I hesitate slightly because of both scope creep and the potential for #228 to take a while to actually be finished and incorporated. For now, sure, go ahead with that. If #228 remains unresolved for more than another week, we should probably come back and separate this.

👍