HajimeKawahara / exojax

🐈 Automatic differentiable spectrum modeling of exoplanets/brown dwarfs using JAX, compatible with NumPyro and JAXopt
http://secondearths.sakura.ne.jp/exojax/
MIT License
45 stars 14 forks source link

`qt_K` -> `qr` in Forward_modeling_for_Fe_I_lines_of_Kurucz.ipynb #487

Open code29563 opened 3 months ago

code29563 commented 3 months ago

I think something missing from this notebook is to divide qt_K determined in cell 5 by adbK.QTref_284[mask] before passing to SijT (or now line_strength) in the next cell, as the function requires qr: partition function ratio qr(T) = Q(T)/Q(Tref) for that argument. Could you confirm?

HajimeKawahara commented 3 months ago

Thanks. @chonma0ctopus Can you also check this?

HajimeKawahara commented 2 months ago

Working on this issue at kurucz_bug branch

chonma0ctopus commented 2 months ago

I apologize for the bug I left behind that has caused you all trouble. After a quick look, I think @code29563 has made the right point. I will take the time to look at it more closely to modify them next Sunday night. Sorry, I may have misunderstood. You say you're already working on it. Thank you. Tell me where I still should clarify and edit.

HajimeKawahara commented 2 months ago

@chonma0ctopus I reached Cell 5 by modifying Cell 1-4 (needed some modifications). Can you take over kurucz_bug branch?

https://github.com/HajimeKawahara/exojax/blob/kurucz_bug/documents/tutorials/Forward_modeling_for_Fe_I_lines_of_Kurucz.ipynb

chonma0ctopus commented 2 months ago

Understood.

HajimeKawahara commented 2 months ago

@chonma0ctopus Have you identified the cause of the error?

chonma0ctopus commented 2 months ago

Sorry for the further delay. I've finally taken the time to manage the issues in the notebook: https://github.com/chonma0ctopus/exojax/tree/kurucz_bug

I see similar problems in a few other files (e.g., Tref assignment issues on line_strength, qt used instead of qr, and some deprecated functions...). I'm planning to tackle these too:

Quick question: Should I go ahead and merge the notebook fixes into your develop branch first? Or wait until I've sorted out the other files too?

HajimeKawahara commented 2 months ago

@chonma0ctopus Thanks! Choose one that suits you best!

chonma0ctopus commented 2 months ago

Got it! I'll work on the other files first next weekend.

chonma0ctopus commented 1 month ago

Thanks for your patience. lpf.py and opacity_Fe_test.py were processed. I will work on the remaining in a week.

HajimeKawahara commented 1 month ago

@chonma0ctopus Hello. Do you think I can help this? or if you already finished correcting all of the source file (except for ipynb/rst), make a PR if you prefer. then I can review.

chonma0ctopus commented 1 month ago

Thank you for offering your help. I apologize for not being able to dedicate time to this. It would be helpful if you could check "opacity_Fe_test.py" because this test seems to show some error even before my edit. Thank you again for your support and understanding.

HajimeKawahara commented 1 month ago

@chonma0ctopus Thanks for the reply. but, I'm a bit confused. Which version of opacity_Fe_test.py should I check? If I should look at the opacity_Fe_test.py you processed, could you create a draft PR or submit a PR?

chonma0ctopus commented 1 month ago

Sorry for the confusion. It was not the version I processed this time, it was giving the error before I edited it. I haven't been able to pinpoint at which point in development the cause was.

I thought it might be at Issue #488, but it wasn't; at 49b9e3283ea64b3cee8197a3ebe058b442b9eaad we already get an AssertionError.

Do you happen to know any smart way to track down at what point the difference occurred? Or, do you think there is no need to track it and rather I can just update the assert statement?

HajimeKawahara commented 1 month ago

I see. But you said you have already processed opacity_Fe_test.py. Making changes based on the old version will make merging later difficult, so could you please submit a PR anyway? If you do that, I will edit the new version of opacity_Fe_test.py. @chonma0ctopus

chonma0ctopus commented 1 month ago

Ok, I've submitted a PR. Thank you in advance for your review.

HajimeKawahara commented 1 month ago

Thanks for the PR. I do not have any error for opacity_Fe_test.py....

shirochan:~/exojax/tests/endtoend/metals(kurucz_bug)>python opacity_Fe_test.py
Reading VALD file
/home/kawahara/exojax/src/exojax/spec/atomllapi.py:612: FutureWarning: Calling float on a single element Series is deprecated and will raise a TypeError in the future. Use float(ser.iloc[0]) instead
  ionE = float(
[2.07535337e-19 1.25000001e-05 2.85156250e-05]
[2.07535337e-19 1.25000001e-05 4.53125000e-04]
[2.07535337e-19 1.25000001e-05 1.16015625e-04]
[2.07535337e-19 1.25000001e-05 2.19726562e-05]
[2.07535337e-19 1.25000001e-05 5.09375000e-04]

@chonma0ctopus

chonma0ctopus commented 1 month ago

Sounds great, but also puzzling... As seen below, the CPU is being used for my calculations because my outdated CUDA/cuDNN settings are not compatible with the latest JAX. Do you think this could be the cause of the difference?

RB15:~/ghR/exojax/tests/endtoend/metals$ python opacity_Fe_test.py 
An NVIDIA GPU may be present on this machine, but a CUDA-enabled jaxlib is not installed. Falling back to cpu.
/home/tako/miniconda3/lib/python3.9/site-packages/numba/__init__.py:143: UserWarning: A NumPy version >=1.22.4 and <2.3.0 is required for this version of SciPy (detected version 1.22.3)
  import scipy
Reading VALD file
Note: Couldn't find the hdf5 format. We convert data to the hdf5 format.
[3.15955554e-19 2.31640625e-04 2.85156250e-05]
[3.15955554e-19 2.31640625e-04 3.87109375e-03]
Traceback (most recent call last):
  File "/home/tako/ghR/exojax/tests/endtoend/metals/opacity_Fe_test.py", line 175, in <module>
    test_opacity_Fe_uns(2995., 0.1)
  File "/home/tako/ghR/exojax/tests/endtoend/metals/opacity_Fe_test.py", line 106, in test_opacity_Fe_uns
    assert(diff[0] < 1.e-11 and diff[1] < 1.e-3 and diff[2] < 1.e-3)
AssertionError
HajimeKawahara commented 1 month ago

something wrong... Did you do python setup.py install? Or remove hdf file and regenerate it? (i.e. gf2600.hdf)

chonma0ctopus commented 1 month ago

Yes, I did both.

chonma0ctopus commented 1 month ago

Just to be sure, could you also try using only the CPU instead of GPU? @HajimeKawahara

HajimeKawahara commented 1 month ago

@chonma0ctopus I did using CPU but the same results:

shirochan:~/exojax/tests/endtoend/metals(kurucz_bug)>python opacity_Fe_test.py 
Reading VALD file
/home/kawahara/exojax/src/exojax/spec/atomllapi.py:612: FutureWarning: Calling float on a single element Series is deprecated and will raise a TypeError in the future. Use float(ser.iloc[0]) instead
  ionE = float(
[2.07535337e-19 1.25000001e-05 2.85156250e-05]
[2.07535337e-19 1.25000001e-05 4.53125000e-04]
[2.07535337e-19 1.25000001e-05 1.16015625e-04]
[2.07535337e-19 1.25000001e-05 2.19726562e-05]
[2.07535337e-19 1.25000001e-05 5.09375000e-04]
HajimeKawahara commented 1 month ago

Anyway I merged #499. But can you continue to identify the issue making another branch? @chonma0ctopus

chonma0ctopus commented 1 month ago

Thank you. I understand!