POSYDON-code / POSYDON

POSYDON is a next-generation single and binary-star population synthesis code incorporating full stellar structure and evolution modeling with the use of MESA.
BSD 3-Clause "New" or "Revised" License
29 stars 19 forks source link

Update absolute import support for ini files. #310

Closed ka-rocha closed 3 months ago

ka-rocha commented 4 months ago

Many people have been trying to run posydon with custom code for many different reasons. Old code for relative imports was not working well so I have updated it so the syntax is cleaner. Here is an example using the new feature:

[step_HMS_HMS]
  import = ['posydon.binary_evol.MESA.step_mesa', 'MS_MS_step']
    # builtin posydon step
  absolute_import =[ '/projects/b1119/karocha/dev_posydon/custom_steps/step_end.py', 'StepEnd']
    # If given, use an absolute filepath to user defined step: ['<PATH_TO_PY_FILE>', '<NAME>']
  ...

If both import and absolute_import are provided, the default behavior is to use absolute_import. If we parse this ini file we get:

from posydon.popsyn.io import simprop_kwargs_from_ini

sim_prop_kwargs = simprop_kwargs_from_ini('./custom_steps/test_rel_path.ini')
for key, val in sim_prop_kwargs.items():
    if key == 'flow': continue
    print(key, val)

Results in the following:

>>> step_HMS_HMS (<class 'projects.b1119.karocha.dev_posydon.custom_steps.step_end.StepEnd'>, {})

Which includes the full path as the module name :)

TODO:

ka-rocha commented 4 months ago

@maxbriel I noticed that when I try to run a simple population with the default ini file it fails because metallicity is a list. Here is an example that breaks:

from posydon.popsyn.binarypopulation import BinaryPopulation

pop = BinaryPopulation.from_ini('population_params_default.ini')
pop.evolve()

What do you think about the default behavior being metallicity as a float and add handling of float vs. multiple metallicities into SyntheticPopulation?

mkruckow commented 4 months ago

Because this absolute import didn't worked before, I have proposed a few weeks ago to add a user_modules directory, so that users can put there code there and use it as part of the posydon name space without touching the main code. It would have taken the user away the need to take care of managing a module installation themselves. With this PR we would go to ask the user to always provide their code in a module maintained by themselves. And PR #296 would be mostly obsolete and I'd need to clean it out/recreate a new PR with the stuff we should keep. A question would be do we want to provide the users an example of an external module to be called from POSYDON, like the modified flow-chart I used in PR #296?

ka-rocha commented 4 months ago

@mkruckow I had not seen the PR#296 before! I like this addition of user_modules for persistent changes to the code that are not default like your custom flow. I do think your additions and the ones introduced in this PR do not necessarily overlap though so I think we should have both PRs.

The user_modules addition is good for persistent code changes that are different from the default but they would require users to have access to and change the source code which is not recommended for conda installs which we want to support in the future. Especially for development of science projects, I think it will also be faster to point to a specific file than editing files in a repo where you may also be doing development work. But with my addition of absolute paths, it is hard to share ini files as people will have their own local directory structure that must be changed so I see pros and cons of both implementations.

So in summary, I think both of our PRs cover useful and different cases for adding custom code to POSYDON. @mkruckow let me know what you think!

mkruckow commented 4 months ago

True, both ways have their pros and cons. I'm fine with having both, we should probably discuss this in the next developer meeting. It might be a bit of a challenge to write a good documentation including both without confusing users.