dynamics-of-stellar-systems / dynamite

dynamics, age and metallicity indicators tracing evolution
MIT License
10 stars 1 forks source link

Fixed a bug in retrofitting kinmapchi2 in old all_models tables. #357

Closed maindlt closed 6 months ago

maindlt commented 7 months ago

When reading an old all_models table which does not have the kinmapchi2 column, the column was added but calculating the kinmapchi2 values did not work (silently...). This is fixed in this PR which - as discussed in a recent hack session - has been extracted from PR #322 for quicker merging. Until then, #322 will remain demoted to draft.

Remark: Separated reading an existing all_models table (AllModels.read_model_table()) from updating it (AllModels.update_model_table()) to avoid problems with the configuration object (config_reader.py takes care of this).

Successfully tested with test_nnls.py and both weight solvers to verify that a deleted kinmapchi2 column gets re-added upon reading the all models table. Also, test_notebooks.sh succeeds.

Test suggestion:

  1. Run the original test_nnls.py.
  2. In test_nnls.py:36, set reset_existing_output=False.
  3. In all_models.ecsv, delete the kinmapchi2 column (headline + 3 values) and its entry in the header (entire line 14).
  4. Re-run test_nnls.py and inspect the output: all_models.ecsv should now have the kinmapchi2 column with the correct values again.

This test should also succeed when changing the weight solver (legacy weight solver <-> Python NNLS) in the config file between deleting and re-adding the kinmapchi2 column (with some warnings because the config file will now differ from the one uses when initially creating all_models.ecsv).

prashjet commented 6 months ago

I ran your test suggestion - all worked as expected.