-
Notifications
You must be signed in to change notification settings - Fork 11
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
New design Heaviside pipeline #99
Conversation
d7b62bd
to
081fc94
Compare
cc9be2b
to
ad8922e
Compare
ad8922e
to
2349d97
Compare
ed92156
to
cca4c62
Compare
There are a few minor bugs I found so far:
|
properties | ||
f_x | ||
|
||
c |
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.
this already exists in base, why here again?
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 don't believe it is in nosnoc.model.Base
. As a matter of fact matlab will loudly complain if you double define a property.
@@ -0,0 +1,46 @@ | |||
classdef Heaviside < nosnoc.model.Base | |||
properties |
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.
Add short comment (1-2 sentences) why is this not a PSS in some cases (will be in docu anyway)
+nosnoc/+dcs/Heaviside.m
Outdated
obj.dims = dims; | ||
end | ||
|
||
function generate_variables_from_heaviside(obj, opts) |
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.
the functions _pss and _heaviside (later also for equaitons) share a lot of code (Except for the theta and f_x parts, they are almost indentical).
What speak against of having one function for vars and eqs, and have a if-else or switch-case?
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.
No extremely strong opinions here. I think in this case we can absolutely have an if statement. The only thing is sometimes it is nice to have a pseudo-linear way to read the code even if it requires some repetition. Perhaps this is an over-correction on my part however, as I have been quite frustrated during this rewrite with the interleaving if
s in the old design and disentangling them one by one.
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 agree, but 90% is repetition and adds quite a few extra lines. Well, most of the old ifs are mostly resolved by having extra classes for every problem type.
I can live with the repetition, but shorter and still understandable code is also not bad.
I am not sure yet how to resolve, if one wants to use PSS with the Heaviside reformulation, but calls the Heaviside model by creating the model object. |
My view on this is RTFM 😅. If a user uses the wrong class without reading even the top level documentation and gets |
This is not a bug per se, but it is the fact that |
What confused even me, that the commented code in the examples was suggesting that this is a valid path. :| |
Hmm well that is definitely something we should avoid then. Strictly speaking the point is that the |
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.
LGTM GJ
No description provided.