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

[WIP] Proposal for the common data reader #1583

Closed
wants to merge 7 commits into from

Conversation

scarlehoff
Copy link
Member

This is a continuation of #1548 with only the absolutely necessary files so that the code review and maintenance during development is a bit easier.

@scarlehoff scarlehoff requested a review from Zaharid August 3, 2022 11:04
@scarlehoff
Copy link
Member Author

@Zaharid the structure of the whole commondata is finished, before continuing, could you have review commondataparser.py up to ##### TODO: the functions below have not been touched yet so they are targetting old commondata to make sure that I'm using validobj correctly, that I'm not doing something stupid or unpythonic (unvalidobjy).

If you could also check pineparser.py (lines 67-73, the rest is just using the object instead of the dictionary) it would be much appreciated

@scarlehoff
Copy link
Member Author

@Zaharid the structure of the whole commondata is finished, before continuing, [...]

@Zaharid did you review this and are super happy with it or you didn´t?



@Parser
def ValidOperation(op_str: str) -> str:
Copy link
Contributor

@Zaharid Zaharid Sep 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could do ValidOPeration = Literal[*list_of_operations].

Edit, unless we insist on allowing "AdD" and similar.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid I don't know what you mean here. How should I write this in validobj language?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Literally my comment. With ‘list_of_operations’=OP or whatever.

@scarlehoff scarlehoff force-pushed the final_reader_for_new_commondata branch from 0cf3087 to e39b513 Compare December 14, 2022 10:42
@scarlehoff
Copy link
Member Author

I'm more or less happy with the reader now.

@enocera @tgiani what is the status of the conversion to the new format? I would like to have a bunch of datasets so that I can truly test that I'm covering all (or most) corner cases.

@enocera
Copy link
Contributor

enocera commented Dec 14, 2022

@scarlehoff You'll likely have DIS+DY after Christmas.

@tgiani
Copy link
Contributor

tgiani commented Dec 14, 2022

yes DY should be finalised shortly after christmas

@scarlehoff
Copy link
Member Author

Ok, in the meantime I’ll give a try to reading the old commondata into the new format (so that libNNPDF can be dropped)



@Parser
def ValidVariants(variant_dict: dict) -> Dict[str, Variant]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed. Can be just the return type and will be processed recursively as done manually in the function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean that it will be able to understand that it is a dict of Variants by itself or the Parser is not needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it should.

arXiv: Optional[ValidReference] = None
iNSPIRE: Optional[ValidReference] = None
hepdata: Optional[ValidReference] = None
variants: Optional[ValidVariants] = dataclasses.field(default_factory=dict)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be optional if the default is not going to be None

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can put the default to None of course. It is just a question of adding a If variants is None in a few places.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can as well make it a dict. But at the moment if you do set it to none it is broken in several places.

@scarlehoff scarlehoff closed this Feb 23, 2023
@scarlehoff scarlehoff deleted the final_reader_for_new_commondata branch February 23, 2023 12:55
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.

None yet

4 participants