reflectivity / orsopy

http://www.reflectometry.org/orsopy/
MIT License
0 stars 8 forks source link

[DOC]: Simple example usage docs #95

Closed arm61 closed 1 year ago

arm61 commented 2 years ago

Add a simple and general example of how to use the orsopy fileio module to produce a valid .ort file

arm61 commented 2 years ago

Need to add something about Column for the code to be "realistic".

arm61 commented 2 years ago

My worry with "just include a few example python scripts" is that (for ESS reduction code) the interaction with orsopy is segmented over many different modules so it can be difficult to follow the thread.

arm61 commented 2 years ago

I still want to make it a better example of "best practice" before I merge. Working on it a bit tomorrow.

arm61 commented 2 years ago

Thoughts on having a from_dict() class method for Orso? This would mean that one can create an empty object, populate it and then do a Orso.from_dict(my_object.to_dict()) to check everything. @aglavic @bmaranville

bmaranville commented 2 years ago

There is already a deserializer functionality in fileio.orso, used by load_orso. How would you envision the new from_dict method would interact with or complement that?

arm61 commented 2 years ago

I didn't know about that functionality. It might be exactly what we need (but it would be a class method of the Orso class).

aglavic commented 1 year ago

@arm61

Thoughts on having a from_dict() class method for Orso? This would mean that one can create an empty object, populate it and then do a Orso.from_dict(my_object.to_dict()) to check everything.

Sorry for being late to the party. For me this method is not needed, as this is already possible/used. You just have to supply the dictionary as argument Orso(**my_object.to_dict()). If the object is already Orso, it makes more sense to me to supply a convenience method to run the check against schema. I think we already have a function for that. If you think it is more intuitive, we could also supply a class method from_dict as you suggest that just runs the code above.

arm61 commented 1 year ago

Finally got back to this, I have added a correctness check at the end. I think this is the most intuitive approach (cause you can tab-autocomplete from the header object without having to know the exact syntax.

arm61 commented 1 year ago

Thoughts on merging this as is with the plan to improve in the future @aglavic @bmaranville ?

bmaranville commented 1 year ago

Do we have a specification for the "physical quantity"? This definition is important to me, but I am not sure where we have written down the "official" list of physical quantity values, like "wavevector transfer" for q.

It would be nice if this list was available in the code somewhere, so that it could be optionally validated (optionally is very important.)

arm61 commented 1 year ago

From the specification (https://github.com/reflectivity/file_format/blob/master/specification.md#column-description) the physical quantity is to be a "plain name" I take that to mean something that the users sees as accurately describing what the column contains.

I am hesitant to be overly prescriptive with respect to what can go in there.

bmaranville commented 1 year ago

physical quantity has been suggested (I think by Jochen?) as a solution to the problem of unambiguously identifying values that might be columns or might be in the measurement.instrument_settings, e.g. wavelength. In the context of that discussion it has been suggested that we formalize the names available: see https://github.com/reflectivity/file_format/blob/master/specs_discussion.md#reserve-key-words

I don't really care if we use physical quantity or something else, but we need some way to identify where to find e.g. wavelength or incident angle in a file, since sometimes that will be a column, and sometimes it will be buried deep in the header, and it's critical information in many cases.

arm61 commented 1 year ago

I agree with the concerns. I suggest we open a different issue for it though instead of getting into the weeds on this PR.

bmaranville commented 1 year ago

You're right - it's not specific to this PR. We can update the usage docs when the issue is resolved.