search

ITA-Solar / helita

A Python library for solar physics from the Institute of Theoretical Astrophysics, University of Oslo
BSD 3-Clause "New" or "Revised" License
9 stars 17 forks source link

Second try #18

Closed jumasy closed 5 years ago

jumasy commented 5 years ago

Bifrost_units now is generic. Ebysus has new format readout. Minor bugs fixed.

tiagopereira commented 5 years ago

This seems mostly fine, although I don't have much time right now to test.

tiagopereira commented 4 years ago

I've just noticed that this introduced several bugs. First, many things now use the units in a way that doesn't work. Calls to uni.* fail, because uni is not defined in many functions. It is a really ugly way to deal with this. Please fix this, @jumasy !

tiagopereira commented 4 years ago

Rhoeetab is also broken.

jumasy commented 4 years ago

Hi Tiago, I did just a new push. I am not sure this fix the problems you mention. Note that Bifrost_units is a class now and allows to use the normalization units from the model which can change. However, I do not think I fixed any bug there. It was working for me and Viggo before and the new version too. Could you send me the issues you have?

tiagopereira commented 4 years ago

The problems I had specifically were in get_hydrogen_pops and get_electron_density, that called uni without uni being defined. I've now replaced it in my copy with a direct self.params['u_ee'] instead of uni.

Rhoeetab is broken because it assumes self.params['abund'] exists, which it doesn't for nearly all Bifrost files I've tried with. I've now fixed that and will push a new update tomorrow.

I see you've added commits to your branch, but unless you do a pull request, I'm unable to merge them. I also don't think they fix these issues I got, so probably not necessary to merge right now.

jumasy commented 4 years ago

Why self.params['abund'] in Rhoeetab? I do not see the point to have it in there. If I'm not wrong, abund is only defined in tabparam.in. So it does not make too much sense. Unless other routines outside bifrost.py uses abund. I suggest to remove this line unless you have other reasons. About get_hydrogen_pops, or get_electron_density should work in my version. Now Bifrost_units is called in init

If you agree, I can remove self.params['abund'] in Rhoeetab and commit and push request.

tiagopereira commented 4 years ago

According to the "blame", you put the following line in Rhoeetab:

self.params['abund'] = 10**(self.params['abund'] - 12.0)

The problem in get_electron_density is that you define uni = Bifrost_units() and then call self.uni.u_ee.

It would be great if you could do a pull request addressing these problems.

jumasy commented 4 years ago

done. I wonder what I was thinking at that time about abund. Thank you Tiago

On Sep 20, 2019, at 1:38 AM, Tiago M. D. Pereira notifications@github.com wrote:

According to the "blame", you put the following line in Rhoeetab:

self.params['abund'] = 10**(self.params['abund'] - 12.0) The problem in get_electron_density is that you define uni = Bifrost_units() and then call self.uni.u_ee.

It would be great if you could do a pull request addressing these problems.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ITA-Solar/helita/pull/18?email_source=notifications&email_token=ADNHQBDQMPK5WS7SHMZAHF3QKSDW5A5CNFSM4H7ODP22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7GAHBY#issuecomment-533463943, or mute the thread https://github.com/notifications/unsubscribe-auth/ADNHQBHLEFLRFPJF6ATGTYTQKSDW5ANCNFSM4H7ODP2Q.