8000 [Draft] Fix DO_TITER option by AlexHls · Pull Request #186 · artis-mcrt/artis · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[Draft] Fix DO_TITER option #186

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

[Draft] Fix DO_TITER option #186

wants to merge 2 commits into from

Conversation

AlexHls
Copy link
Member
@AlexHls AlexHls commented Apr 18, 2025

In the current develop branch, enabling the DO_TITER does not seem to work. Superficially, this is a compile issue, but I'm not sure if there are some deeper issues.

So far I've been able to fix the compile issues and get it to run with 3 iterations for timesteps after the first timestep (i.e. setting globals::n_titer = (globals::timestep > 1) ? 3 : 1;), but my test set 8000 up crashes later for some other reason I haven't been able to figure out. Somebody more knowledgable than me may need to run some verification tests to make sure this works as intended.

@AlexHls AlexHls added the bug label Apr 18, 2025
@AlexHls AlexHls requested a review from fionntancallan April 18, 2025 14:21
@AlexHls AlexHls requested a review from lukeshingles as a code owner April 18, 2025 14:21
@lukeshingles
Copy link
Member
lukeshingles commented Apr 18, 2025

I probably won’t get to this properly until April 28, but can you tell me more about why you need this feature? If there’s a need for it, we should add a test to the CI to ensure it stays in a working state. I personally only used it for a one-off experiment many years ago.

@AlexHls
Copy link
Member Author
AlexHls commented Apr 18, 2025

We're currently running some experiments including a pure H/He envelope. In those cases the ionization stage seems to jump in between timesteps (in case of H from mostly neutral to fully ionizes), causing issues in e.g. the electron temperature etc. @ssim suggested that using multiple iterations per timestep may help to converge the ionization stage (given that artis lacks the infrastructure to limit the maximum change in ionization).

Assuming that this solves our issues, I would have a more long-term need for this feature as several of my models would involve such envelopes.

@lukeshingles
Copy link
Member
lukeshingles commented Apr 18, 2025

Ok! If this is going to be useful, I suggest we add another artisoptions.h function, something like:

constexpr int NUM_TIMESTEP_ITERATIONS(int timestep) { return (timestep <= -1) ? 3 : 1; }

Then this can be enabled for one of the CI tests (maybe in classicmode_1d) and we'll store the checksums. Hopefully you're using classic options? The timestep iteration stuff isn't implemented for the binned radiation field or the detailed bound-free estimators that we use for NLTE calculations.

@AlexHls
Copy link
Member Author
AlexHls commented Apr 18, 2025

Hopefully you're using classic options? The timestep iteration stuff isn't implemented for the binned radiation field or the detailed bound-free estimators that we use for NLTE calculations.

Ah, this explains why it runs in my test setup (classic), but not in my production setup, which runs on NLTE settings (or at least a binned radiation field) since we're mostly interested in nebular spectra. Is this something that could be implemented rather easily or would this require some major code changes? I'm willing to do most of the heavy lifting here, at least as far as my expertise goes.

@lukeshingles
Copy link
Member

Yes, I think you could do it with a bit of work. If you happen to come up with a neat way to do this (i.e. not just duplicating all of the arrays with a ‘_save’ suffix) that would be even better, but even the current way extended to those extra features would be fine to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0