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

refactor stress components (introducing OptimalStress_Szz) #14

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Thomas-Ulrich
Copy link
Contributor

Following Carsten's review on PR#13, I realized that if I want to rewrite OptimalStress to prescribe s_zz and not Tr(s), I won't be able to recover the exact output of the old OptimalStress, even with a FunctionMap.
(you may think that I m quibbling, but James is starting to work on normal fault, and I think this change can make a difference).
Therefore, I propose to introduce a new OptimalStress_Szz component, which prescribes s_zz, and to keep OptimalStress, but to clearly state in the docu that it is deprecated.
With that, I'm transforming easi into a cementary of deprecated stress components, but I'm not breaking backward compatibility (e.g. to run the SC17 and 21 papers).
Let me know what you think.

@uphoffc
Copy link
Contributor

uphoffc commented Jul 16, 2021

I think we should introduce version numbers.

@Thomas-Ulrich
Copy link
Contributor Author

and drop the old stress components?
I'm not sure I see how that would work.

@uphoffc
Copy link
Contributor

uphoffc commented Jul 16, 2021

No you keep both optimal stress components, say OptimalStress and OptimalStress_Szz. If an old version or no version is selected you would register OptimalStress for !OptimalStress. If a new version is selected you register OptimalStress_Szz for !OptimalStress.

@krenzland
Copy link
Contributor

The proper way of doing this would be to provide version numbers (using e.g. semver) for easi/SeisSol. State for which version numbers each setup work. Then, first deprecate old features (still work, but give out warning), after some time drop support.
There is no need for backwards compatibility imo. Setups continue to work with the older versions. You only need to update when you want to use new features

@Thomas-Ulrich
Copy link
Contributor Author

How to deal with the warning if the stress component is called million of times in a run?

@uphoffc
Copy link
Contributor

uphoffc commented Jul 19, 2021

But in this case it wouldn't be a big deal to add backward compatibility? I agree that a version doesn't need to be backward compatible to all previous versions, but here its easy, so I wouldn't drop it.

Any suggestions of where to put a version number in the yaml files? I think we'd need some kind of header, because everything is a component and I wouldn't add version numbers of components.

Suggestion:

version: 0.1.0
components: !BlaMap
  <components as usual>

Any thoughts?

PS: Sorry Thomas for making such a big deal out of this, but I guess its better if we resolve this now.

@uphoffc
Copy link
Contributor

uphoffc commented Jul 19, 2021

How to deal with the warning if the stress component is called million of times in a run?

One only prints it once when parsing the file.

@Thomas-Ulrich
Copy link
Contributor Author

adding the version tag would then break all previous setup?
(I mean we would need to add the version in all yaml files?).

@uphoffc
Copy link
Contributor

uphoffc commented Jul 19, 2021

No, one could just check whether the version tag is there or not. If it is not there, then the old behaviour is used.

@Thomas-Ulrich
Copy link
Contributor Author

ok, then I think it would be great!
(hopefully not overkill for my little stress routines).

@krenzland
Copy link
Contributor

Uhm, should we add a version tag to the easi file itself? I think this is going to be very confusing. Especially because we do not have any versioning concept currently...

@uphoffc
Copy link
Contributor

uphoffc commented Jul 19, 2021

How else do you want to make changes but be still backward compatible to published setups?

@Thomas-Ulrich Thomas-Ulrich marked this pull request as draft September 30, 2024 06:24
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