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

Use super in get_structure #44

Merged
merged 1 commit into from
Mar 16, 2021
Merged

Use super in get_structure #44

merged 1 commit into from
Mar 16, 2021

Conversation

pmrv
Copy link
Contributor

@pmrv pmrv commented Jan 24, 2021

Use super instead of hardcoding GenericInteractive. I just want to run the tests on this.

@coveralls
Copy link

coveralls commented Jan 24, 2021

Pull Request Test Coverage Report for Build 656936926

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 64.876%

Totals Coverage Status
Change from base Build 642350783: 0.0%
Covered Lines: 10735
Relevant Lines: 16547

💛 - Coveralls

@pmrv pmrv marked this pull request as ready for review February 4, 2021 09:19
@pmrv
Copy link
Contributor Author

pmrv commented Feb 4, 2021

The codacy nag comes from them not considering multiple inheritance issues. Since we want to bypass the get_structure() from LammpsBase and AtomisticGenericJob and instead want the one from GenericInteractive, this is the correct way to call super().

@niklassiemer
Copy link
Member

Then, of course, one may ask if our hierarchy is correctly set up since we have to use the method defined multiple layers above...

@pmrv
Copy link
Contributor Author

pmrv commented Feb 4, 2021

In the case of multiple inheritance like we have it (see below) it's necessary, since python's default mro is depth-first, but we want breadth first here.

AtomisticGeneric
       |
LammpsBase    GenericInteracive
       |                 |
       \                 /
       LammpsInteractive 

That said, since all our lammps jobs are actually LammpsInteractive under the hood, restructuring would be possible.

@jan-janssen
Copy link
Member

jan-janssen commented Feb 12, 2021

I just looked at the pull request now, from my perspective it is a bit more complex:

            GenericJob
            /         \
            |         |
AtomisticGeneric      |
            |      GenericInteracive
     LammpsBase       |   
            |         |
            \        /
       LammpsInteractive 

A possible way to resolve this would be:

 GenericJob
      |
GenericInteracive
      | 
AtomisticGeneric
      | 
LammpsBase
      | 
LammpsInteractive 

This makes a lot of sense when the static jobs also support interactive execution, meaning calculating energies and forces for multiple structures, which should definitely be a goal.

@stale
Copy link

stale bot commented Feb 27, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 27, 2021
@stale stale bot closed this Mar 14, 2021
@pmrv pmrv reopened this Mar 15, 2021
@stale stale bot removed the stale label Mar 15, 2021
@pmrv
Copy link
Contributor Author

pmrv commented Mar 16, 2021

Since it's unlikely that we restructure soon and everything seems to work, I'll merge this as soon as the notebook tests pass.

@pmrv pmrv merged commit 6071c63 into master Mar 16, 2021
@delete-merged-branch delete-merged-branch bot deleted the get_structure_super branch March 16, 2021 09:59
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