Initial item/file domain detectors

[robertbartel]

Adding initial set of implementations of classes that can scan a data item/file and detect the applicable DataDomain for its data.

Note that this branch builds off that of #564. As such, this PR should remain in draft status until #564 is merged.

[aaraney]

I recommend that we drop this in favor of subtypes defining their own initializer that ideally is the only initializer (no need to call super().__init__()). In doing so, subtypes can more freely specify their requirements and loosen the coupling between the super and subclass.

[robertbartel]

Hmm ... I'm not really making use of it, but I had wanted a lazily initialized property for accessing the domain value, and for all the detector types to have this. My concern was if a detector needed to be used repeatedly (for the same data) that it'd be a performance hit to have to re-detect things over and over. Though at this point I'm not sure though how often, if ever, that scenario is actually going to come up.

Is that - complications from introducing a common, inherited property implementation - specifically what you are worried about here, or is there something more?

[aaraney]

but I had wanted a lazily initialized property for accessing the domain value, and for all the detector types to have this. My concern was if a detector needed to be used repeatedly (for the same data) that it'd be a performance hit to have to re-detect things over and over.

Yeah, I like that functionality and in some if not all cases that is desirable. My thinking is that subclasses can handle caching / lazy loading in their detect implementation. It likely will result in a minor amount of code duplication or at least code that looks repetitive, however I think considering the tradeoffs of having to know the relationship between the parent class and the subclass or a few lines of repetitive code I would rather have as much of the behavior in one place. Another, at least to me, interesting philosophical question. What are your thoughts?

[aaraney]

I get that this is a nicety, but imo there is nothing stopping a subclass from implementing this behavior in their implementation of detect. This also places the burden on subtypes to update _data_domain when detect is called. I think the design can be simplified by dropping this property and the _data_domain class level instance variable. This could result in slightly more code, but the behavior of a subtype will be closer to its declaration.

[robertbartel]

I get that this is a nicety, but imo there is nothing stopping a subclass from implementing this behavior in their implementation of detect.

True. I think my motivation is that there is nothing making them either, nor making all of them do it in a consistent manner (e.g., if you want to re-detect versus return what was detected already, can you do that, and is it done the same way?). That seemed most easily solvable by doing some things in the superclass.

This also places the burden on subtypes to update _data_domain when detect is called.

Actually the reverse is true. Calling detect is intended not to impact _data_domain. This is why data_domain and reset are implemented: to control (and return) the _data_domain value (though, as I mentioned above, I think I'm not actually using them at the moment). But detect should run detection and return a new object each time.

I think the design can be simplified by dropping this property and the _data_domain class level instance variable. This could result in slightly more code, but the behavior of a subtype will be closer to its declaration.

Heh, we're in another thing where my style shows itself to not be especially Pythonic.

I'd intended to leverage the superclass to abstract a concern from the subclass implementations (though if we stick with that, I do need to at least go back and use the property in places). We could skip that, and just have the independent detect definitions. Subclasses would be more free to handle themselves, but I think we'd get slightly less predictable and consistent implementations. I'm a little worried that, given how we're using these in places like the universal detector, that inconsistency could eventually become a problem, even if it isn't right now.

[aaraney]

Heh, we're in another thing where my style shows itself to not be especially Pythonic.

Haha, I don't think your style is not Pythonic. I just tend to have an aversion to inheritance outside of using it strictly for enforcing an interface. Especially, when you start sharing private properties between inheritance levels. So, for sure a style thing!

I'd intended to leverage the superclass to abstract a concern from the subclass implementations (though if we stick with that, I do need to at least go back and use the property in places). We could skip that, and just have the independent detect definitions. Subclasses would be more free to handle themselves, but I think we'd get slightly less predictable and consistent implementations. I'm a little worried that, given how we're using these in places like the universal detector, that inconsistency could eventually become a problem, even if it isn't right now.

First off, thanks for articulating your design so throughly! I agree that abstracting the "caching" of the detected DataDomain in a consistent way is valuable. I think we can achieve this behavior, in a similar fashion, without requiring the functionality be provided by the base class in a consistent way by using composition instead. This has the benefit of decoupling the "caching" behavior from the detectors and provides a separate pathway to add complexity to "caching" mechanisms if needed in the future without having to muck around with how each AbstractDomainDetector works. What that could look like in practice is:

class CachedDomainDetector:
  def __init__(self, detector: AbstractDomainDetector):
    self._detector = detector
    self._data_domain = None

  @property
  def data_domain(self) -> DataDomain:
     if self._data_domain is None:
       self._data_domain = self._detector.detect()
     return self._data_domain

  def reset(self):
    self._data_domain = None

Alternatively, it could be a subclass of AbstractDomainDetector. So, something like:

class CachedDomainDetector(AbstractDomainDetector):
  def __init__(self, detector: AbstractDomainDetector):
    self._detector = detector
    self._data_domain = None

  def detect(self, **kwargs) -> DataDomain:
     if self._data_domain is None:
       self._data_domain = self._detector.detect()
     return self._data_domain

  def reset(self):
    self._data_domain = None

In either case, we are able to move the base class prescribed behavior elsewhere and decouple the behavioral concepts. The trade off is now you have to know about whatever that caching wrapper type is, but I would argue that once you find it, its far easier to understand the implications of that abstraction rather than having to walk up the inheritance tree and see how each parent type handles the, in this case, _data_domain property.

[robertbartel]

I went ahead and just took it out. I'm still not certain there wouldn't be complications moving to a composition like this that aren't worth the trouble for something as straightforward as a lazy property, but we didn't actually need it. And we can always add the composition in the future if we later decide to.

[robertbartel]

@aaraney, I think all the changes we've discussed have been made. I'm a little puzzled by the test failures. I'm not seeing them locally, and I created a fresh 3.10 environment to test in just to be safe. I'm curious if you see these on your side. FWIW, I did clear the cache entries related to this PR, and it didn't seem to help.

Update
Tests are passing now. Best guess: the built packages were cached, and I hadn't updated the versions and dependency pinnings as needed, so old dependencies were being used.

[robertbartel]

@aaraney, @christophertubbs, is there anything remaining for this PR that needs to be changed before it can be approved?

[aaraney]

I think we are getting really close to being at a point to merge this. I left a few small comments that will likely turn into TODOs. Let's briefly continue the discussion, I don't want to hold this up much longer.

[aaraney]

Can we improve the type hint experience here a little? Perhaps something like:

Suggested change

	def detect(self, **kwargs) -> DataDomain:
	def detect(
	self,
	*,
	excluded_formats: Optional[Union[DataFormat, Set[DataFormat]]] = None,
	suggested_formats: Optional[Union[DataFormat, List[DataFormat]]] = None,
	sort_key: Optional[Callable[[Type[ItemDataDomainDetector]], T]] = None,
	**kwargs
	) -> DataDomain: ...

[aaraney]

My above point about setting ._instance in __init__ is moot, if this stays present. Im on the fence. Ill leave it to your discretion if you think the change is worth it.

[aaraney]

Since this does not make use of any optional keyword arguments, I would recommend that we change the signature to the following to indicate that.

Suggested change

	def detect(self, **kwargs) -> DataDomain:
	def detect(self, **_) -> DataDomain:

[aaraney]

We will also want to handle / ignore any csv comment headers. So, for example lines that start with #.

[aaraney]

pandas.DataFrame.iloc is going to return a pandas.Series and that is not exactly what we want. I suggest we use pandas.DataFrame.at instead.

Suggested change

	date_range = TimeRange(begin=df.iloc[0][dt_index].to_pydatetime(), end=df.iloc[-1][dt_index].to_pydatetime())
	date_range = TimeRange(begin=df.at[df.index[0], dt_index].to_pydatetime(), end=df.at[df.index[-1], dt_index].to_pydatetime())

[aaraney]

Like your TODO says, I don't think we need this.

https://geopandas.org/en/stable/docs/reference/api/geopandas.read_file.html

filename
str, path object or file-like object
Either the absolute or relative path to the file or URL to be opened, or any object with a read() method (such as an open file or StringIO)

[aaraney]

It looks like some VPUs have a directional suffix:

nextgen_01.gpkg
nextgen_02.gpkg
nextgen_03N.gpkg
nextgen_03S.gpkg
nextgen_03W.gpkg
nextgen_04.gpkg
nextgen_05.gpkg
nextgen_06.gpkg
nextgen_07.gpkg
nextgen_08.gpkg
nextgen_09.gpkg
nextgen_10L.gpkg
nextgen_10U.gpkg
nextgen_11.gpkg
nextgen_12.gpkg
nextgen_13.gpkg
nextgen_14.gpkg
nextgen_15.gpkg
nextgen_16.gpkg
nextgen_17.gpkg
nextgen_18.gpkg

[aaraney]

I'm seeing that a lot of the comments I've made here have already been addressed in #566. Let's move this along and merge this.