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

Spx log parser #1222

Merged
merged 22 commits into from
Nov 28, 2023
Merged

Spx log parser #1222

merged 22 commits into from
Nov 28, 2023

Conversation

samwaseda
Copy link
Member

This is the first one in the series of changes to be made in order for the Sphinx parsers to become independent.

@coveralls
Copy link

coveralls commented Nov 28, 2023

Pull Request Test Coverage Report for Build 7017344019

  • 126 of 155 (81.29%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 68.577%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyiron_atomistics/sphinx/output_parser.py 121 150 80.67%
Totals Coverage Status
Change from base Build 7003678790: -0.01%
Covered Lines: 10735
Relevant Lines: 15654

💛 - Coveralls

@samwaseda samwaseda added enhancement New feature or request format_black reformat the code using the black standard labels Nov 28, 2023
@samwaseda samwaseda changed the title Spx output parser Spx log parser Nov 28, 2023
@samwaseda samwaseda added format_black reformat the code using the black standard and removed format_black reformat the code using the black standard labels Nov 28, 2023
@samwaseda
Copy link
Member Author

@skatnagallu This is the first parser that I would like to hand over to you. Since the class does not return a dictionary, as @jan-janssen and I had agreed in a recent workshop, but instead returns a full class, we might want to make changes in the future, but otherwise I would in principle leave the parser to you

@jan-janssen
Copy link
Member

@skatnagallu This is the first parser that I would like to hand over to you. Since the class does not return a dictionary, as @jan-janssen and I had agreed in a recent workshop, but instead returns a full class, we might want to make changes in the future, but otherwise I would in principle leave the parser to you

@samwaseda I would prefer to do the migration to the dictionary now, otherwise changes in the class might directly break the code in pyiron_atomistics. Having a well defined dictionary as interface seems to be more clean to me.

@samwaseda
Copy link
Member Author

@samwaseda I would prefer to do the migration to the dictionary now, otherwise changes in the class might directly break the code in pyiron_atomistics. Having a well defined dictionary as interface seems to be more clean to me.

Definitely! But I would still separate the two PRs

@jan-janssen
Copy link
Member

@samwaseda I would prefer to do the migration to the dictionary now, otherwise changes in the class might directly break the code in pyiron_atomistics. Having a well defined dictionary as interface seems to be more clean to me.

Definitely! But I would still separate the two PRs

Ok, that is fine with me as well.

@samwaseda
Copy link
Member Author

Maybe I'm gonna rephrase my comment: I feel a strong responsibility to separate the Sphinx parsers from pyiron, because I don't think anyone can / wants to clean up the mess there. But as soon as it's cleanly separated, I don't feel a strong responsibility to make the change from class properties & getter functions to a dictionary, because we are on an equal footing after the separation. This being said, if you guys have the feeling that I should do it, I can definitely take care of it, since it should be done relatively soon in the future.

@skatnagallu
Copy link
Contributor

@skatnagallu This is the first parser that I would like to hand over to you. Since the class does not return a dictionary, as @jan-janssen and I had agreed in a recent workshop, but instead returns a full class, we might want to make changes in the future, but otherwise I would in principle leave the parser to you

Sounds good. I am not sure if I understand what the dictionary should be? So the entire parsed file should be output as a dictionary with corresponding properties? I can do this in a separate PR.

@samwaseda
Copy link
Member Author

Sounds good. I am not sure if I understand what the dictionary should be? So the entire parsed file should be output as a dictionary with corresponding properties? I can do this in a separate PR.

Yep that’s a very good point. I have the feeling that in the case of SphinxLogParser there are different possibilities, so I guess it’s worth talking about it with everyone who uses SPHInX. For a general idea, I will suggest it in the upcoming PR’s, after this one is merged.

@samwaseda
Copy link
Member Author

Can I merge this one?

@skatnagallu
Copy link
Contributor

yes, I think so.

@samwaseda samwaseda added the integration Start the notebook integration tests for this PR label Nov 28, 2023
@samwaseda samwaseda merged commit 4d36d6b into main Nov 28, 2023
28 of 29 checks passed
@delete-merged-branch delete-merged-branch bot deleted the spx_output_parser branch November 28, 2023 14:22
@samwaseda
Copy link
Member Author

@samwaseda I would prefer to do the migration to the dictionary now, otherwise changes in the class might directly break the code in pyiron_atomistics. Having a well defined dictionary as interface seems to be more clean to me.

I ended up doing it here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request format_black reformat the code using the black standard integration Start the notebook integration tests for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants