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

Flow Separation Warning #205

Merged
merged 1 commit into from
May 30, 2023

Conversation

AstroChuck
Copy link
Contributor

If the simulation shows an exit pressure which is below the Summerfield Criteria for flow separation, a warning will be generated.

Copy link
Owner

@reilleya reilleya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea! Two small things to tweak.

motorlib/nozzle.py Outdated Show resolved Hide resolved
motorlib/motor.py Outdated Show resolved Hide resolved
@AstroChuck
Copy link
Contributor Author

AstroChuck commented Jan 17, 2023

Both changes have been implemented. I also added some import logic so older files will have the correct default separation ratio. I also added a fix where the start and end data points would trigger the warning, so the warning now only triggers if the condition persists.
I also recalled you were trying to keep the commits clean, so I squashed this commit with the previous one so the PR only has a single commit.

@petterreinholdtsen
Copy link

The test build of this patch fail. Perhaps something to look at?

Copy link
Owner

@reilleya reilleya left a 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. One more tweak. Also, I fixed the issue with the tests on staging so you can rebase and this should pass.

uilib/fileIO.py Outdated Show resolved Hide resolved
motorlib/simResult.py Outdated Show resolved Hide resolved
@AstroChuck AstroChuck force-pushed the nozzle_separation_warning branch 2 times, most recently from 452c1d7 to c9d70b2 Compare February 25, 2023 23:00
@AstroChuck
Copy link
Contributor Author

All new requested changes have been implemented.

@reilleya
Copy link
Owner

I finally got around to playing with this and really like it! Last issue is that the warning text is too long, at least on my laptop:
image
I know what it says so it isn't a problem for me, but it seems like we should either find a more concise message, make this window resizable, or at least make it wider. I'd be happy to merge it with that changed!

@benrussell11
Copy link

benrussell11 commented May 19, 2023 via email

@reilleya
Copy link
Owner

Not quite! It will be once this is merged, which will happen as soon as we touch up the text in the warning.

If the simulation shows an exit pressure which is below the Summerfield Criteria for flow separation, a warning will be generated.

Implementing Requested Changes

Nozzle Flow Separation Warning now triggers on a % threshold rather than on any value. Threshold is adjustable.

Updated Warning message

warning message now shorter to fit smaller screens.
@AstroChuck
Copy link
Contributor Author

I've made the requested change, the warning text is now approximately the same length as the other warnings. A longer term future improvement seems to be warranted to improve the warning message GUI to support more verbose and informative messages, but I'm going to put a pin in that for now.

@AstroChuck AstroChuck requested a review from reilleya May 29, 2023 01:28
@reilleya reilleya merged commit 1286748 into reilleya:staging May 30, 2023
3 checks passed
@AstroChuck AstroChuck deleted the nozzle_separation_warning branch May 30, 2023 04:43
@benrussell11
Copy link

benrussell11 commented May 30, 2023 via email

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.

None yet

4 participants