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

Extend parser to include specialized EntryData sections #131

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ka-sarthak
Copy link
Collaborator

@ka-sarthak ka-sarthak commented Oct 9, 2024

The definition of the EntryData class that is generated by the MatchingParser has been abstracted out into an attribute entrydata_definition. This is set in the set_entrydata_definition method.

Benefit:

  • In a situation where a specialized EntryData section is defined for the XRD entry, the XRDParser can be reused by overwriting set_entrydata_definition method. For example:
class SpecializedELNXRayDiffraction(ELNXRayDiffraction):
 # additional quantities
 def normalize(self, archive, logger):
   # additional normalization
   pass

class SpecializedXRDParser(XRDParser):
 def set_entrydata_definition(self):
   self.entrydata_definition = SpecializedELNXRayDiffraction

@ka-sarthak ka-sarthak force-pushed the allow-extension-of-parser branch 2 times, most recently from 4922265 to 7432c3f Compare October 9, 2024 10:16
@ka-sarthak
Copy link
Collaborator Author

@aalbino2 can you check if this works in your case?

@ka-sarthak ka-sarthak requested a review from aalbino2 October 9, 2024 10:57
Copy link
Contributor

@aalbino2 aalbino2 left a comment

Choose a reason for hiding this comment

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

I will implement this solution in a parser that will be included in nomad-measurements soon. I think this can be merged. Thanks for figuring out a solution that will be useful for all the parsers.

@aalbino2
Copy link
Contributor

hey @ka-sarthak, apparently @JonathanNoky cannot make this work. he need to call the function inside the parse function:

   def parse(self, mainfile: str, archive: EntryArchive, logger) -> None:
        self.set_entrydata_definition()

what do you think? shouldn't it be already executed without calling it?

or do we need to really include it into the init method that is always run?

Indeed the linter says "W0201:attribute-defined-outside-init"

@JonathanNoky
Copy link
Collaborator

Hey @ka-sarthak , after looking into your example again I figured it out. I missed the part where the set_entrydata_definition is called at the beginning of parse().
It is working now.

@ka-sarthak
Copy link
Collaborator Author

The idea is to override the setter function that sets the entrydata definition. In a specialized MatchingParser class, you override this setter function and that's it.

@aalbino2 you are right, self.entrydata_definition is being defined outside the init method in this case. Maybe we can make it an attribute of the parent parser and update it in the child's parse method. Like:

class XRDParser(MatchingParser):
  entrydata_definition: str = ELNXRayDiffraction
  def parse(self):
    ...

class SpecializedXRDParser(XRDParser):
  def parse(self):
    self.entrydata_definition = SpecializedELNXRayDiffraction
    super().parse()

@aalbino2
Copy link
Contributor

The idea is to override the setter function that sets the entrydata definition. In a specialized MatchingParser class, you override this setter function and that's it.

@aalbino2 you are right, self.entrydata_definition is being defined outside the init method in this case. Maybe we can make it an attribute of the parent parser and update it in the child's parse method. Like:

class XRDParser(MatchingParser):
  entrydata_definition: str = ELNXRayDiffraction
  def parse(self):
    ...

class SpecializedXRDParser(XRDParser):
  def parse(self):
    self.entrydata_definition = SpecializedELNXRayDiffraction
    super().parse()

okay, I think this alternative can make sense.

@JonathanNoky will you try this solution in ppms instead of the one previously proposed?

@ka-sarthak
Copy link
Collaborator Author

I haven't tested it, so please take it with a grain of salt

@JonathanNoky
Copy link
Collaborator

Yes, this one also works. Its shorter than the other method, as there is just one line in the parse function.

@JonathanNoky
Copy link
Collaborator

Actually, after some thinking about it, I think, the method of just setting the attribute in the parse function does not work for more nested parser classes like this:

  entrydata_definition: str = ELNXRayDiffraction
  def parse(self):
    ...

class SpecializedXRDParser(XRDParser):
  def parse(self):
    self.entrydata_definition = SpecializedELNXRayDiffraction
    super().parse()

class EvenMoreSpecializedXRDParser(SpecializedXRDParser):
  def parse(self):
    self.entrydata_definition = EvenMoreSpecializedELNXRayDiffraction
    super().parse()

Here, the EvenMoreSpecializedXRDParser would via the super().parse() function still get the SpecializedELNXRayDiffraction defintion, right?

So I think, I will have to go back to the first proposed solution with the separate definition function. I will try it in my CPFSPPMSParser plugin, as this has this kind of deeper nested parsers when I let it depend on the nomad-measurements plugin.

@ka-sarthak
Copy link
Collaborator Author

@JonathanNoky That's a good point. Having a setter function allows for more flexibility and deeper nesting.

@ka-sarthak ka-sarthak self-assigned this Jan 8, 2025
@ka-sarthak ka-sarthak force-pushed the allow-extension-of-parser branch from c57766e to e1f4237 Compare January 8, 2025 15:09
@ka-sarthak ka-sarthak force-pushed the allow-extension-of-parser branch from e1f4237 to 131e9c2 Compare January 8, 2025 16:13
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.

3 participants