search

mabuchilab / Instrumental

Python-based instrumentation library from the Mabuchi Lab.
http://instrumental-lib.readthedocs.org/
GNU General Public License v3.0
117 stars 77 forks source link

Facet validators #126

Closed NitramC closed 3 years ago

NitramC commented 3 years ago

Hi Nate,

I tried to port over the "validator" concept from pymeasure. It seems like a fairly straight forward and extensible way to validate user input. Perhaps even things like check_enums could be rolled into it, but that seems a bit complicated at the moment...

I tried to preserve all the functionality of the previous Facet. I have no experience with unit testing, but it sounds like it would be a good way to test if I have broken anything... Do you have any old unit tests?

While I was implementing validators, I also tried to make the get and set methods/algorithms as close to mirrors of each other as possible. It seemed like it could be easier for newbies to understand the code if getting was almost the inverse of setting -- understand one, and you understand the other for free.

Sorry the pull request is a bit messy... It took me a while to appreciate the complexity of dealing with unitful quantities and caching their values. I also needed to get up to speed with python properties, which is why the getter/setter were deleted then readded :)

It would be great if you could shed some light on the purpose of the FacetData class. Could this data be stored in an instance of Facet instead? Or, maybe, should all the instance variables of Facet be stored in a FacetData instance?

-Martin

NitramC commented 3 years ago

Let me just clean this all up and make a new pull request.

natezb commented 3 years ago

Hi Martin, sorry I'm only getting around to this now. It's been awhile since I've thought much about this stuff, so it'll take a bit to refresh my cache.

FacetData is necessary because of how Python descriptors (like Facet and property) work, specifically that instances of Facet are associated with an Instrument class, not an instance. That's why __get__() requires the obj parameter.

So, a FacetData instance is used to contain the per-instrument-instance data associated with a given facet.

NitramC commented 3 years ago

Thanks for clarifying that. It is all starting to make more sense... I can see now why you were suggesting using FacetData for storing dynamic limits. I think that is a good idea.