-
Notifications
You must be signed in to change notification settings - Fork 15
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
Tchipev fixes mixing rules #76
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this ugly lines distributed on many spots in the code should be removed completely. Mixing rules parameters should be read only via XML and be stored at only one reasonable location in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should get rid of this check in Simulation.cpp completely. There should be a check at the location directly when reading in the mixing rule parameters, if they are corrupted like to less or to many values specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think that this is correct. Should be symmetric.
…s for mixing rules.
@tchipev any update on this? |
Hi, I've modified the changes accordingly, but not pushed them yet. I need to test them and can then commit. Will try to get to it today or tomorrow. |
OK, should be good now. |
the tests are failing for this PR. Can you adapt them? |
Sorry, I don't have the code in my head anymore. Could you please take over? I think I added more safety checks and changed the program to abort in case of dubious input (when only some mixing rules are specified). This, however, breaks backwards compatibility which were the failing checks, if I remember correctly. Since this breaks backwards compatibility, maybe you can briefly discuss it with application users. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not mergable as tests have to be adapted
Hey! I saw that! :)
…On Thu, Jan 28, 2021 at 4:59 PM Steffen Seckler ***@***.***> wrote:
Closed #76 <#76>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#76 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFRCNJWT4F3EU7V4F4GALF3S4GCWHANCNFSM4HFTQXLA>
.
|
Fix #74 and #75 and many of #73 .