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

[Core][Element/Condition] using const #4938

Merged
merged 12 commits into from
Jul 22, 2019
Merged

[Core][Element/Condition] using const #4938

merged 12 commits into from
Jul 22, 2019

Conversation

philbucher
Copy link
Member

this is a new, BACKWARD COMPATIBLE attempt to #2993, for now only a draft and not all the changes to discuss if this is the way to go

=> adv: Backwardcompatible, disadv: the compilation is full of deprecation warnings. Updating the code in the derived classes is easy but at least for some days we would have many warnings

Tests seem to still be working, so I guess this solution would be ok

One way to go would be to add the deprecation-warnings only after some time.
This would give the developers time to adapt

@philbucher philbucher requested review from jcotela and a team May 23, 2019 09:52
@philbucher philbucher self-assigned this May 23, 2019
@philbucher philbucher marked this pull request as ready for review May 30, 2019 14:04
@philbucher
Copy link
Member Author

@KratosMultiphysics/technical-committee This is ready to be reviewed

Note that the new functions are in most places NOT being called, since the scheme still passes on a non-const ProcessInfo and hence directly the old functions will be called
The schemes will be adapted in the next PR, after this the developers can start adapting their elements/conditions

@philbucher
Copy link
Member Author

@maceligueta it seems like there already is a function Initialize in a DEM-element:

/home/travis/build/KratosMultiphysics/Kratos/applications/DEMApplication/custom_elements/spheric_particle.h:134:14: error: ‘virtual void Kratos::SphericParticle::Initialize(const Kratos::ProcessInfo&)’ can be marked override [-Werror=suggest-override]
 virtual void Initialize(const ProcessInfo& r_process_info);

how is this currently used?

@philbucher
Copy link
Member Author

Ping @maceligueta @KratosMultiphysics/dem

@philbucher
Copy link
Member Author

ping @maceligueta @KratosMultiphysics/dem

@maceligueta
Copy link
Member

Hi @philbucher , sorry about the delay. Many years ago we decided to create an Initialize method with the ProcessInfo inside. The elements call this function after they are created. However, as you can imagine, we needed to downcast in order to call this function (and a few others). The fact that the Discrete Elements are so different from Finite Elements was limiting our options very much, so we opted for the downcasting, knowing that it's not an ideal solution. Neither it is computing discrete elements with a Finite Elements code.
But right now this is a good change for us, we can override the new method and loop over Elements instead of looping over SphericParticles. Not a big gain for us, because we do the downcasting for other methods, but still going in a better direction.
I don't know if this answers your question...

@philbucher
Copy link
Member Author

@maceligueta thanks, this is what I suspected
I guess I can just add the override then (this is why travis fails) or do you have concerns this would not work?

@maceligueta
Copy link
Member

No concerns. I think it should work if you just add the override.

@philbucher
Copy link
Member Author

It seems to work now
Ping @KratosMultiphysics/technical-committee

@@ -131,7 +131,7 @@ void TransformNeighbourCoorsToClosestInPeriodicDomain(const ProcessInfo& r_proce
virtual bool CalculateRelativePositionsOrSkipContact(ParticleDataBuffer & data_buffer);

using DiscreteElement::Initialize; //To avoid Clang Warning. We tell the compiler that we are aware of the existence of this function, but we overload it still.
Copy link
Member Author

Choose a reason for hiding this comment

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

@maceligueta I guess this can be removed afterwards

Copy link
Member

Choose a reason for hiding this comment

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

sure! Should I?

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 think it is better to do it in a future PR, this one is already large and contains delicate changes

@philbucher
Copy link
Member Author

@KratosMultiphysics/technical-committee now that we have sorted out some larger things, I would like to take a look at this again, since I think this is quite a necessary cleanup.

@pooyan-dadvand
Copy link
Member

@KratosMultiphysics/technical-committee: 👍

@pooyan-dadvand
Copy link
Member

Just for the record:

This will be followed by another PR changing the scheme to use the added methods with const ProcessInfo and then all developers should adapt their own elements and conditions.

Copy link
Member

@pooyan-dadvand pooyan-dadvand left a comment

Choose a reason for hiding this comment

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

👍

@philbucher
Copy link
Member Author

merging such that I can proceed with the porting

@philbucher philbucher merged commit 3cae235 into master Jul 22, 2019
@philbucher philbucher deleted the core/elem-cond-const branch July 22, 2019 07:06
@philbucher
Copy link
Member Author

please wait with implementing this in your code, there will be a followup-PR, with which this PR will become fully available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants