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

Change mass density to number density #88

Open
pcubillos opened this issue Jul 20, 2015 · 3 comments
Open

Change mass density to number density #88

pcubillos opened this issue Jul 20, 2015 · 3 comments
Assignees

Comments

@pcubillos
Copy link
Member

Nowhere in the code the mass density (g cm-3) is used 'as is'. A more natural unit is the number density.
This will simplify the code, and make it more standard with the units used in the literature (e.g., the units in the opacity file will be cm2/mol instead of cm2/g).

@pcubillos pcubillos self-assigned this Jul 20, 2015
@joeharr4
Copy link
Contributor

Where in the code is the density used? Is it always converted to number
density wherever it is used? How big a change is this, and does it
affect just config files or C code? While in principle this change
looks like a good idea, it has "bug" written all over it, if you forget
even one instance that needs changing. This is the kind of thing where
tests help!

--jh--

X-Spam-Status: No, score=-4.8 required=4.0 tests=BAYES_00,HTML_IMAGE_ONLY_12,
HTML_MESSAGE,RCVD_IN_DNSWL_HI,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,
T_DKIM_INVALID autolearn=ham autolearn_force=no version=3.4.0
Date: Mon, 20 Jul 2015 08:08:22 -0700
From: Patricio Cubillos [email protected]
Reply-To: exosports/transit [email protected]
To: exosports/transit [email protected]
Subject: [transit] Change mass density to number density (#88)
Content-Type: multipart/alternative;
boundary="--==_mimepart_55ad0ee6aea35_14f93fa5532df2c098319";
charset=UTF-8

[1:text/plain Hide]

Nowhere in the code the mass density (g cm-3) is used 'as is'. A more natural unit is the number density.
This will simplify the code, and make it more standard with the units used in the literature (e.g., the units in the opacity file will be cm2/mol instead of cm2/g).


Reply to this email directly or view it on GitHub:
#88

[2:text/html Show]

@pcubillos
Copy link
Member Author

Where in the code is the density used?
Many places.

Is it always converted to number
density wherever it is used?
Yes, always.

How big a change is this, and does it
affect just config files or C code?
The other way around, it affect C code but not any config files. I would say it deals with some 4-8 source files, a few structure definitions, a many places in the documentation.

While in principle this change
looks like a good idea, it has "bug" written all over it, if you forget
even one instance that needs changing. This is the kind of thing where
tests help!
I know, but I also think this is an important change. We want others to use this code. Making a new paradigm to handle a problem that others have already solved must be justified (which is not the case here). I will make things easier and more welcoming for others when they want to use Transit, it will make things easier to us when we want to compare against other results from the literature.
This is a risk I'm willing to take.

@joeharr4
Copy link
Contributor

I know, but I also think this is an important change. We want others
to use this code. Making a new paradigm to handle a problem that
others have already solved must be justified (which is not the case
here). I will make things easier and more welcoming for others when
they want to use Transit, it will make things easier to us when we
want to compare against other results from the literature.

This is a risk I'm willing to take.

I am wondering why Pato Rojo did it that way in the first place. Did
you ask? But, I agree it's a good idea to do this. The question is
when, who, and how to ensure it's done right.

--jh--

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants