-
Notifications
You must be signed in to change notification settings - Fork 1
Fire plume update #161
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
Fire plume update #161
Conversation
…sens/fire-plume/tree/master, added and modified config settings
|
Hi @martinjanssens really nice that you're able to work with this! Let me know if you need help/input or would like a review at some point. |
…same Line as the background (so it follows the same color cycle on permutations), just dashed
…entrainment. The entrainment factor is a pure guess for now.
|
Right, thanks for the positive response, @Peter9192 :) It was quite okay working in the code. I changed quite a lot around the fire model to get the points above to work, so am happy if you look it over. I think I did most of it right, as I'm not breaking anything in my own tests. I'm not totally sure whether I robustly solved plotting fire plumes: I had transposed the plume output to get it in similar form as CLASS output, so I could rescale the x-axis for large plume values. But when making the Plume line plotting function consistent with that output, I think I broke it, so I removed it and am now just using line plots. For the future, I need to think about the surface-layer entrainment model, as it's now entirely arbitrary. But this should at least make the fire bit ready for the training in October. |
Peter9192
left a comment
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.
Hi again, really good progress, and I understand from Chiel that the plumes look more realistic now. I'll focus on the code, not the physics.
I approved CI runs for this PR, you can see that they're still failing. One is a type error, I commented on that - easy fix. The other is from a test:
| test("given emtpy object should return default config", () => { |
Since you've updated some defaults in the config, we need to also modify the expected values in the test. This way of testing is not ideal, I agree, but at least it's better than nothing.
Concerning the transpose / back transpose, I think you may be losing some fire plume specific formatting (dashed red line). I might look into how you could fix the back transpose later. It might be good to think in general how we would like these plumes to be plotted.
One thing that does seem to break is the following: if I load a case e.g. Santa Coloma, and change the settings to activate the fire plume, it shows up fine. But if I then toggle to wind speed, I get an error. I think it might be because you're axes scaling implementation now tries to read from plume data, but that's not available for wind. You may need to make that dependent on whether showPlume is true or false
…d protected non-existing fire plume variables behind a check for these variables
|
That's really helpful. Thank you. I've fixed the two easy problems I think, and have tried to sort out the plotting of variables that don't exist for the plume when the plume plotting is toggled, by placing the generation of fire plume profile data behind a Regarding the plume-specific formatting: I've now just made all fire plumes appear in dashed lines in the same color as the CLASS environment they were launched in. So if you plot 3 permutations in the same plot with the fire plume toggled, we now get 3 environments and 3 dashed lines, where each plume and environment have the same color. This works logically for me in all permutations I can think of, including those that only modify the fire. |
Fireplume update - also see #161
|
Nice work again! Sorry for the delay. I fixed the type issue in 3f34214. The 'normal' way to do this is by using a type guard (notice the Merged via #163, thanks for the contribution! |
|
@martinjanssens I've invited you as maintainer for the original repo so next time you can commit/push directly to that one instead of working via your own fork |
This should ideally make us ready for the October training. Still need to: