reilleya / openMotor

An open-source internal ballistics simulator for rocket motor experimenters
GNU General Public License v3.0
400 stars 78 forks source link

ENUM refactor #216

Open Flavsditz opened 1 year ago

Flavsditz commented 1 year ago

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 the uilib 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.

Flavsditz commented 1 year ago

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

benrussell11 commented 1 year ago

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 https://github.com/reilleya/openMotor/pull/216#issuecomment-1586096655 , 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: @. @.> >

Flavsditz commented 1 year ago

@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?

benrussell11 commented 1 year ago

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 https://github.com/reilleya/openMotor/pull/216#issuecomment-1587840415 , 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: @. @.> >

reilleya commented 1 year ago

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 uilib/fileIO.py for the preference files if all that changed was the specific strings used as keys/values. Here are my preferences and propellants (renamed from .yaml for uploading) as a reference, let me know if you have any questions on how to write the migration.

preferences.txt propellants.txt

Flavsditz commented 1 year ago

You bet … let me know when you need my preference file and propellant files.

Hey @benrussell11 feel free to send me right away :) Having two real-world examples is definitely helpful

benrussell11 commented 1 year ago

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 https://github.com/reilleya/openMotor/pull/216#issuecomment-1589892212 , 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: (mPa)/s: (inpsi)/s N: N Ns: Ns Pa: psi kg: lb kg/(m^2s): lb/(in^2s) kg/m^3: lb/in^3 kg/s: lb/s m: in m/(sPa): m/(sPa) m/(sPa^n): in/(spsi^n) m/s: ft/s m^3: in^3 type: !!python/object/apply:uilib.fileIO.fileTypes

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

benrussell11 commented 1 year ago

You bet … let me know when you need my preference file and propellant files.

Hey @benrussell11 feel free to send me right away :) Having two real-world examples is definitely helpful

Here you go and thanks for helping out. preferences.txt requirements.txt

Flavsditz commented 1 year ago

Thanks for the files both of you! (@benrussell11 you sent a requirements.txt file instead of the propellant file though ^^ but it doesn't matter... I think I had enough to pinpoint the issue) Well I added a fix.

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 !!python/object/apply:uilib.fileIO.fileTypes which doesn't exist anymore and I simply did a search and replace by replacing with !!python/object/apply:uilib.enums.fileType.FileType.

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?