-
Notifications
You must be signed in to change notification settings - Fork 74
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
ENUM refactor #216
base: staging
Are you sure you want to change the base?
ENUM refactor #216
Conversation
…file and updated deps
I seem to notice that these changes are not backward compatible with the old preferences file... It would throw an error if you try to update it... |
As a user, definitely not ok with having to recreate my preferences or propellant files. As a user, the system will need to convert from the old to the new.
Ben
From: Flavsditz ***@***.***>
Sent: Sunday, June 11, 2023 5:56 AM
To: reilleya/openMotor ***@***.***>
Cc: Subscribed ***@***.***>
Subject: Re: [reilleya/openMotor] ENUM refactor (PR #216)
I seem to notice that these changes are not backward compatible with the old preferences file... It would throw an error if you try to update it...
I am looking if there is a way to offer backwards compatibility. This doesn't block a discussion but probably the merge unless we are ok of saying people need to erase their old preferences and propellants files
—
Reply to this email directly, view it on GitHub <#216 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AL4HO6E7ZP6GXWMI2U5YLL3XKWIZTANCNFSM6AAAAAAZCFPHNA> .
You are receiving this because you are subscribed to this thread. <https://github.com/notifications/beacon/AL4HO6FHSH6E6UUG2Z54SYTXKWIZTA5CNFSM6AAAAAAZCFPHNCWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTS6RHVA6.gif> Message ID: ***@***.*** ***@***.***> >
|
@benrussell11 yes I thought so and I have a couple of ideas how to deal with this that I am studying... |
You bet … let me know when you need my preference file and propellant files.
Ben
From: Flavsditz ***@***.***>
Sent: Monday, June 12, 2023 2:17 PM
To: reilleya/openMotor ***@***.***>
Cc: benrussell11 ***@***.***>; Mention ***@***.***>
Subject: Re: [reilleya/openMotor] ENUM refactor (PR #216)
@benrussell11 <https://github.com/benrussell11> yes I thought so and I have a couple of ideas how to deal with this that I am studying...
Would you be able to provide some files so I can have some real-life comparisons?
—
Reply to this email directly, view it on GitHub <#216 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AL4HO6H7BYMIHHEIU3DJXXLXK5MJBANCNFSM6AAAAAAZCFPHNA> .
You are receiving this because you were mentioned. <https://github.com/notifications/beacon/AL4HO6B33P2UAKMKAMFAXYLXK5MJBA5CNFSM6AAAAAAZCFPHNCWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTS6USCZ6.gif> Message ID: ***@***.*** ***@***.***> >
|
Thanks for this substantial PR! I will absolutely find time to look it over this week. In the meantime, I think it should be possible to write a migration in |
Hey @benrussell11 feel free to send me right away :) Having two real-world examples is definitely helpful |
I hope this works. If it doesn’t I’ll add them directly.
Ben
From: Flavsditz ***@***.***>
Sent: Tuesday, June 13, 2023 3:23 PM
To: reilleya/openMotor ***@***.***>
Cc: benrussell11 ***@***.***>; Mention ***@***.***>
Subject: Re: [reilleya/openMotor] ENUM refactor (PR #216)
You bet … let me know when you need my preference file and propellant files.
Hey @benrussell11 <https://github.com/benrussell11> feel free to send me right away :) Having two real-world examples is definitely helpful
—
Reply to this email directly, view it on GitHub <#216 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AL4HO6BYE2FVYJODTQRBTBLXLC423ANCNFSM6AAAAAAZCFPHNA> .
You are receiving this because you were mentioned. <https://github.com/notifications/beacon/AL4HO6HDSXVDIIPAHCVB633XLC423A5CNFSM6AAAAAAZCFPHNCWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTS6YPKHI.gif> Message ID: ***@***.*** ***@***.***> >
data:
general:
ambPressure: 101324.99674500001
burnoutThrustThres: 0.1
burnoutWebThres: 0.0007620015240030481
mapDim: 750
maxMassFlux: 1406.4697609001405
maxPressure: 27580000.000000004
minPortThroat: 2.0
timestep: 0.02
units:
(m*Pa)/s: (in*psi)/s
N: N
Ns: Ns
Pa: psi
kg: lb
kg/(m^2*s): lb/(in^2*s)
kg/m^3: lb/in^3
kg/s: lb/s
m: in
m/(s*Pa): m/(s*Pa)
m/(s*Pa^n): in/(s*psi^n)
m/s: ft/s
m^3: in^3
type: !!python/object/apply:uilib.fileIO.fileTypes
- 1
version: !!python/tuple
- 0
- 5
- 0
appdirs
cycler
decorator
docopt
ezdxf
imageio
matplotlib
networkx
numpy
Pillow
pyparsing
pyqt-distutils
PyQt5
PyQt5-sip
python-dateutil
PyYAML
scikit-fmm
scikit-image
scipy
six
sphinx
|
Here you go and thanks for helping out. |
Thanks for the files both of you! (@benrussell11 you sent a This was happening upon calling the pyYAML constructor so I couldn't solve it by checking the version number of the file. (btw nice solution there!). The problem in itself was just a single ENUM (fileType) where it was trying to find Now this works. It might look a bit hacky and, in hindsight, I could have gone the route of using feature flags properly. I can still do that but it would take more time to check what was changed and also add somewhat a good number of extra code for an undefined amount of time (we can never guarantee everyone is migrated). So I like to consider my solution as a "data migration" if you will ^^ Btw, I noticed that the preferences file contains the version, but it won't update until the user actually open the preferences window... so in theory a user which is very satisfied with their preferences will never open that file and might remain indefinitely on an older version of the file. Might it be worth to save it on exit? |
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.
Thank you so much for taking on this effort! Looks great for the most part, just a couple of minor suggestions.
|
||
# Python 3.11 supports `StrEnum` that would make this a bit more concise to write | ||
# https://docs.python.org/3/library/enum.html#enum.StrEnum | ||
class InhibitedEnds(str, Enum): |
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 try to take this opportunity to make a migration that converts "top"->"forward" and "bottom"->"aft".
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.
As far as I can see this would only be present on the .ric
files. Is this assumption correct?
So that the necessary migration can be done I don't want to miss anything.
Any chance for a more complex .ric
file to be made available so I can use it for testing too?
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.
Sorry for the delay! Here's a kind of silly example that uses all 4 inhibitor configurations:
example.ric.txt
and what it looks like on my end, so you can verify nothing changed:
This is done to tackle the ENUM refactoring as per this discussion.
I have gone through the code and found several entities that could be transformed into ENUMs and done so.
I have created a folder for the ENUMs so it is easy to find and maintain them. There are two, one in the
motorlib
and one in theuilib
since the latter contains ENUMs that are only used there.There are a lot of files changed, I apologize, but there were mentions throughout the code. Now at least a typo wouldn't introduce a problem.
I would like to discuss the "Unit Enums" I have created: Initially, I thought it would be a good idea to keep them separate into their own areas but that led to some files containing 1 or 2 units only. While this is not bad (it is not like we are paying by the file here), one could also argue to just have 1 single ENUM called
Unit
and have them all there.I would like to hear your opinion and I am happy to refactor otherwise.