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

Unable to add disk radiation on restart #320

Closed
Higginbottom opened this issue Jan 16, 2018 · 18 comments
Closed

Unable to add disk radiation on restart #320

Higginbottom opened this issue Jan 16, 2018 · 18 comments
Labels

Comments

@Higginbottom
Copy link
Collaborator

It appears to be currently impossible to 'turn on' a non-emitting disk after restart.

For my rad-hydro work, I want to be able to turn a disk on, after running my zeus-python simulations for some time in order to estimate the effect of permitting wind radiation (which would mainly be produced from the high density disk-like region at the mid plane) to interact with the acceleration zone and outflowing wind. I had already discovered that it is not possible to add a disk into a restarted model, but had hoped that it was possible to run with a non-radiating disk and then turn it on. However this is also apparently not possible. I don't think this is a bug per-se, but perhaps an enhancement.

For simplicity, I've reproduced the issue using the cv_standard parameter file. To reproruce

1: Run the attached parameter file - should take less than a minute...

cv_standard.pf.txt

Note: I think its interesting that if you look at the output from running this, you see lines like

!! xdefine_phot: lum_tot 0.00e+00 lum_star 0.00e+00 lum_disk 0.00e+00 lum_bl 0.00e+00 lum_agn 0.00e+00 lum_wind 0.00e+00

I don't think it is affecting anything, but it is alarming to see the total luminosity reported as zero, with no star lum....

2: Modify the cv_standard.pf file by changing the number of ionisation cycles to 2, and the disk from

Disk.radiation(y=1) 0

to

Disk.radiation(y=1) 1

run with py82d -r cv_standard.pf

one now sees lines like

!! xdefine_phot: lum_tot 0.00e+00 lum_star 0.00e+00 lum_disk 0.00e+00 lum_bl 0.00e+00 lum_agn 0.00e+00 lum_wind 1.54e+34

still oddly no lum_star, but no lum_disk..

As a comparison, if one now just runs the modified cv_standard.pf file from scratch, i.e. 2 cycles with a disk radiating from the start you see

!! xdefine_phot: lum_tot 0.00e+00 lum_star 0.00e+00 lum_disk 4.41e+34 lum_bl 0.00e+00 lum_agn 0.00e+00 lum_wind 1.77e+34

Nick

@Higginbottom
Copy link
Collaborator Author

I just checkout out commit

b7f3abf

After the initial run - i get

!! xdefine_phot: lum_tot 0.00e+00 lum_star 8.94e+32 lum_disk 0.00e+00 lum_bl 0.00e+00 lum_agn 0.00e+00 lum_wind 0.00e+00

So the star is reported correctly - although lum tot is wrong - this is really a different problem isn't it!!

@kslong kslong added bug Bugs in the code domain labels Jan 16, 2018
@kslong
Copy link
Collaborator

kslong commented Jan 16, 2018

Philosophically, what ksl has understood a restart to be is a continuation of an old model with no changes, in contrast to starting a new run with an old windfile (that is with current .pf files system_type = previous. There I thought one should be able to change the radiation sources, in this case the disk.

I believe this is the way things worked in the distant past.

However, at present, a quick check reveals that this option also fails to read in new disk options, that is the routines in setup_disk are never called. Most probably this is due to the way we have rearranged the logic in python.c, which is quite complex.

To fix this problem, we (probably me) needs to look more carefully at the logic, and we (collectively) need to clarify what precisely we would like to be able to change.

I would like to keep the restart option the same as is now, i.e. that we really just use it to continue an old model run.

Here are the files to reproduce the error when one uses uses system type=previous

bug.zip

@Higginbottom
Copy link
Collaborator Author

With the py82 commit i mentioned above, upon restart I get

!! xdefine_phot: lum_tot 0.00e+00 lum_star 8.94e+32 lum_disk 4.41e+34 lum_bl 0.00e+00 lum_agn 0.00e+00 lum_wind 1.54e+34

So the disk can be turned on. This supports your comments Knox, that the change came about with the changes to the inputs that were done in October. I guess this is a classic example of unintended consequences!!!

@kslong
Copy link
Collaborator

kslong commented Jan 16, 2018

At the end of the day, here is where I stand on this. I have created a new remote branch called restart, and made various modifications there. I succeeded in fixing the problem of not being able to turn on radiation in the case where geo.system_type is Previous, but found/created two other problems.

First, both on the dev branch (which I did not change), it does not look like the -t option is working.
I cannot stop a run by setting this to a small number. It would be interesting to know @Higginbottom or @jhmatthews whether you regularly use this option.

And on the restart branch, the -r option is failing in a way that suggests I am losing the windsave information. This is probably because I have not got the logic quite right.

Aside: we have two very similar variables
geo.system_type
geo.run_type

They are needed because geo.system_type has more options (star,binary,agn) than geo.run_type, which just tells one whether this is a new, old, or restarted run.

@Higginbottom
Copy link
Collaborator Author

I don't ever use the -t option.

@jhmatthews
Copy link
Collaborator

I also haven't used it

@kslong
Copy link
Collaborator

kslong commented Jan 17, 2018

I have uploaded a new version of python on the restart branch to address this issue. This branch restores the functionality originally intended for the -r option, which was to complete a previous calculation where the program had been stopped for some reason. With this option, the only thing that one should really change if anything in the .pf file is the number of cycles and maybe the number of photons. Currently, there are a few more variables that are read in, but there are not guarantees this will work.

More importantly though, this version, gives the user fairly complete access to the radiation sources, when you use system_type = previous. In this case, you are asked for all of the parameters that describe the central object and the disk and the associated radiation types etc.

So if you want to start with a model which has the disk turned off, and then turn it on later, here is the procedure:

  • Run the first model as you normally would, using say my_model.pf
  • Copy the .pf file produced for the first model to a new file, viz cp mymodel.out.pf rad_mod.pf
  • Chance the system type to Previous (3) and turn radiation in the disk on
  • Run rad_mod.pf and answer the questions sensibly

In creating this version of the code, I've modified a good bit of the logic in when various rdpar parameters are accessed (and a bit more could be done) so there is a possibility I've messed something up.

@Higginbottom you will need to check whether your various ways of running zeus models still work. You created ways to run those models that are more or less independent from the variables, geo.system_type and geo.run_type. i.e using modes.zeus_connect. Everything would be easier to maintain with out this bypass, but for now fixing this is probably not worth it if it still works as you intended. At any rate, please test what I have done, both for zeus and for the disk radiation question, and let me know whether everything works. At that point, one of us can merge this branch into dev.

@Higginbottom
Copy link
Collaborator Author

Higginbottom commented Jan 18, 2018 via email

@kslong
Copy link
Collaborator

kslong commented Jan 18, 2018 via email

@kslong
Copy link
Collaborator

kslong commented Jan 19, 2018

@Higginbottom I can reproduce the error with the regression test you sent for hydro in the Regression Challenge #292 so until I have fixed that, there is nothing you need to do but continue writing.

@Higginbottom
Copy link
Collaborator Author

Ah! Was just about to post a couple of files. It runs fine just normally - its the -z option that makes it crash!

hdf000002nsh_dw44.zeu.txt
input.pf.txt

kslong added a commit that referenced this issue Jan 21, 2018
…file

is always read in when the -z flag is invoked.  This fixes the segmentation
problem in #320
@kslong
Copy link
Collaborator

kslong commented Jan 21, 2018

@Higginbottom I now have a version of python on the restart branch that does not seg fault. However, you still need to test that it works as you expected, particularly on restarts.

@Higginbottom
Copy link
Collaborator Author

Still crashes for me on restart unfortunately.
input.pf.txt
hdf000004nsh_dw44.zeu.txt

do py82 -z input

then edit in put to do 2 ionisation cycles and do

py82 -z -r input

crash.....

@Higginbottom
Copy link
Collaborator Author

This was for commit:

!!Git commit hash 53336a7

@kslong
Copy link
Collaborator

kslong commented Jan 22, 2018

I see. This is in a different place than the example that I was using, which was from your regression test. It's now failing in ds_to_cone. My guess is that the ConePtrs have not been set up properly.

kslong added a commit that referenced this issue Jan 22, 2018
the rtheta_cones for rtheta models are recalculated on repeats of zeus.

Note that the rtheta_cones which are used to set the boundaries of cells
in rtheta coordinates are not part of wmain, but are separate. These are
not windsaved.  It is possible these should be incoporated into wmain.
@kslong
Copy link
Collaborator

kslong commented Jan 22, 2018

OK. The problem this time was that the rtheta_make_wind_cones was not part of the hydro_restart routine. Wind_cones are not part of wmain and so are not windsaved; hence they must be recalculated. I have added this call to hydro_restart and I was able to run Nick's example, though only @Higginbottom can check that the answers are correct. (Interestingly the segfault only appeared on the Mac, not on linux, which is a bit odd.)

@Higginbottom
Copy link
Collaborator Author

Will test today - but I originally saw it on Iridis, which is a Linux machine....

kslong added a commit that referenced this issue Jan 26, 2018
@kslong
Copy link
Collaborator

kslong commented Jan 26, 2018

Nick confirmed that this restarts and runs beginning with previous models now work as expected.
In a pure restart, there is very little that one can change except the number of cycles.
In a run starting with a previous model, one can change much more, including changing the radiation sources, such as the disk.

@kslong kslong closed this as completed Jan 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants