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

[ParticleMechanics] Python modules init #3689

Merged
merged 6 commits into from
Dec 20, 2018

Conversation

bodhinandach
Copy link
Member

@bodhinandach bodhinandach added Applications FastPR This Pr is simple and / or has been already tested and the revision should be fast labels Dec 18, 2018
@@ -1,5 +1,5 @@
import KratosMultiphysics
import KratosMultiphysics.ParticleMechanicsApplication
import KratosMultiphysics.ParticleMechanicsApplication as KratosParticle
Copy link
Member

Choose a reason for hiding this comment

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

why do you change this? This is not related to the python modules

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a minor cleanup, to make all the import called as KratosParticle. Not related yes.

Copy link
Member

Choose a reason for hiding this comment

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

I checked the file, you are not using anything from the ParticlesApp there, therfore I recommend removing them (also codacy complains abt it being unused which is true)

I recommend to only import them if you need them

Copy link
Member

Choose a reason for hiding this comment

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

we can discuss in person if you have questions

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 agree with you. In fact, I removed it from some other python files too. Thanks for the suggestion!

Copy link
Member

Choose a reason for hiding this comment

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

It is not correct yet, I will explain it to you in person tmr
Nothing big, don't worry

@@ -1,7 +1,7 @@
from __future__ import print_function, absolute_import, division #makes KratosMultiphysics backward compatible with python 2.6 and 2.7

import KratosMultiphysics
import KratosMultiphysics.ParticleMechanicsApplication
import KratosMultiphysics.ParticleMechanicsApplication as KratosParticle
from particle_mechanics_analysis import ParticleMechanicsAnalysis
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from particle_mechanics_analysis import ParticleMechanicsAnalysis
from KratosMultiphysics.ParticleMechanicsApplication.particle_mechanics_analysis import ParticleMechanicsAnalysis

this is what became possible now with the python-modules

@bodhinandach
Copy link
Member Author

@philbucher can you recheck?

@@ -13,6 +12,7 @@

# Importing the base class
from analysis_stage import AnalysisStage
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from analysis_stage import AnalysisStage
from KratosMultiphysics.analysis_stage import AnalysisStage

@philbucher
Copy link
Member

One last thing then its ok

If you find some more then you can fix them in later PRs

@bodhinandach
Copy link
Member Author

Should I change all the import from KratosMultiphysics similarly, or just the analysis stage is fine?

@philbucher
Copy link
Member

Should I change all the import from KratosMultiphysics similarly, or just the analysis stage is fine?

eventually yes, but for now it is ok
you don't have to do it now

@bodhinandach bodhinandach merged commit 5c10c30 into master Dec 20, 2018
@bodhinandach bodhinandach deleted the particle/feature-python-modules branch December 20, 2018 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Applications FastPR This Pr is simple and / or has been already tested and the revision should be fast
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants