Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds mymdc techniques annotation #338

Merged
merged 5 commits into from
Oct 2, 2024
Merged

Adds mymdc techniques annotation #338

merged 5 commits into from
Oct 2, 2024

Conversation

tmichela
Copy link
Member

This adds support for requesting the techniques information from Mymdc, once @RobertRosca deploys the path in ZWOP.

A run may have multiple techniques associated to it. Here I made the choice to return a single string with all techniques rather than a list, as I think most case will only contain a unique technique (as a mater of fact, the DAQ currently support applying only one technique to a run), and it is easier to handle on the user side. But we can change that if you disagree.

Copy link

codecov bot commented Sep 18, 2024

Codecov Report

Attention: Patch coverage is 97.67442% with 1 line in your changes missing coverage. Please review.

Project coverage is 75.35%. Comparing base (1beceb7) to head (b402ec8).
Report is 30 commits behind head on master.

Files with missing lines Patch % Lines
damnit/ctxsupport/ctxrunner.py 97.67% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #338      +/-   ##
==========================================
+ Coverage   74.82%   75.35%   +0.53%     
==========================================
  Files          32       32              
  Lines        4893     4974      +81     
==========================================
+ Hits         3661     3748      +87     
+ Misses       1232     1226       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tmichela
Copy link
Member Author

We could also just add a single argument mymdc#techniques returning a list containing name, identifier, and url for all techniques. Not sure what's the most useful

@takluyver
Copy link
Member

How about we offer the singular forms, technique_name & technique_identifier (or id, as a commonly understood shorthand 🤷 ) for now, either taking only the first one, or validating that there's exactly 1 technique. Then if people start recording multiple techniques for a run, we can add the plural forms, to return lists.

I don't feel strongly about this, but if there's usually 1 technique, it's easy to miss that you're getting a comma-separated list and write something like if technique == 'WAXS' when if 'WAXS' in techniques is more like what you mean.

@tmichela
Copy link
Member Author

if there's usually 1 technique

That's the key question here, and I don't know the answer to it. And since it's new I think it's not used yet (maybe @dallanto has an idea?).
But singular makes sense, unless having more than one technique per run is not unusual.
Alternatively, we could have a simple techniques returning a list of dict (just the information as it is stored on mymdc).

@takluyver
Copy link
Member

I think it's not unusual for FXE to do two types of measurement in the same run - they definitely have many proposals with two techniques, e.g. XES & XAS, and I think the aim is generally to do both simultaneously.

I found runs in p900463 with techniques recorded, including one 'Test DAQ' run with 2 techniques: r203.

So maybe it is worth doing the plural version. In that case I'd go with lists, or a list. I don't have a strong preference between one list-of-dicts vs lists-of-strings. Other than the name & PaNET identifier, I guess you might conceivably want the myMdC ID, and maybe the PaNET URL if you're going to get all semantic web with it. Neither seem especially likely, though I guess myMdC could grow new fields in the future.

@tmichela
Copy link
Member Author

I've changed it for now to return the entire technique information as it is stored in mymdc (list of dicts), that's probably the most versatile and clean option. One can easily iterate over elements and get the information required.
I considered returning an object, that would directly render nicely if returned directly, and would offer easy way of accessing the data, but I think I don't like that idea too much.

@dallanto
Copy link

Hi Thomas & Thomas, "entire information" like all techniques assigned to a given run, e.g. 2 as in the 900463:r203 case?
I agree to this: then "techniques" in plural and list of dicts.

@takluyver
Copy link
Member

LGTM, thanks

@tmichela
Copy link
Member Author

tmichela commented Oct 1, 2024

Thanks, I'll merge once ZWOP is updated (@RobertRosca 👀 )

@RobertRosca
Copy link
Member

So the techniques endpoint isn't needed, only the /runs/{id} endpoint, right? If so then you can test this with the staging instance at https://exfldadev01.desy.de/zwop-staging/

@tmichela tmichela merged commit b211df3 into master Oct 2, 2024
5 checks passed
@tmichela tmichela deleted the feat/mymdc_techniques branch October 2, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants