PLATYPOS issueshttps://gitlab.aip.de/lketzer/platypos/-/issues2020-06-30T06:53:58Zhttps://gitlab.aip.de/lketzer/platypos/-/issues/1DOCUMENTATION: add markdown links in the readme files2020-06-30T06:53:58ZYori Fourniery.fournier@aip.deDOCUMENTATION: add markdown links in the readme filesIt would be very nice to have the link to the ADS entry when a paper is cited.
> This was done accordingly to Poppenhaeger et. al. (2020) [^Poppenhaeger-et-al-20], and we improved it while mixing with Tu et al. (2015) [^Tu-et-al-20]. Yo...It would be very nice to have the link to the ADS entry when a paper is cited.
> This was done accordingly to Poppenhaeger et. al. (2020) [^Poppenhaeger-et-al-20], and we improved it while mixing with Tu et al. (2015) [^Tu-et-al-20]. You can also find ...
[^Poppenhaeger-et-al-20]: [Poppenhaeger et. al. 2020](https://arxiv.org/abs/2005.10240)
[^Tu-et-al-20]: [Tu et. al. 2020](https://arxiv.org/abs/2005.10240)
This is done like that:
```
This was done accordingly to Poppenhaeger et. al. (2020) [^Poppenhaeger-et-al-20],
and we improved it while mixing with Tu et al. (2015) [^Tu-et-al-20]. You can also find ...
[^Poppenhaeger-et-al-20]: [Poppenhaeger et. al. 2020](https://arxiv.org/abs/2005.10240)
[^Tu-et-al-20]: [Tu et. al. 2020](https://arxiv.org/abs/2005.10240)
```Fix all of Yori's comments.Laura KetzerLaura Ketzerhttps://gitlab.aip.de/lketzer/platypos/-/issues/2CLEANING/FEATURE: setup.py and requirements.txt are both empty2020-07-06T08:22:10ZYori Fourniery.fournier@aip.deCLEANING/FEATURE: setup.py and requirements.txt are both emptyThe repository is structured alike a package and examples together. One could decide to separate both but that is fine. However this is not a package yet, since setup.py is empty.
So either we remove setup.py or we make one and import t...The repository is structured alike a package and examples together. One could decide to separate both but that is fine. However this is not a package yet, since setup.py is empty.
So either we remove setup.py or we make one and import the package properly.
Moreover requirements.txt is empty, this could be a great start before making a package.
In both cases I can offer my help. (I have some template for setup.py and requirements.txt is quite easy to do).Fix all of Yori's comments.Laura KetzerLaura Ketzerhttps://gitlab.aip.de/lketzer/platypos/-/issues/3CLEANING: some files should not be there2020-06-26T08:35:05ZYori Fourniery.fournier@aip.deCLEANING: some files should not be thereSome files should not be there:
* `AIP_Cluster_instructions.txt`
* `Email_Notifications.ipynb`
You could add them into `.gitignore`
Moreover versioning `jupyter notebooks` is not a good idea. Except you always take care of stopping th...Some files should not be there:
* `AIP_Cluster_instructions.txt`
* `Email_Notifications.ipynb`
You could add them into `.gitignore`
Moreover versioning `jupyter notebooks` is not a good idea. Except you always take care of stopping the kernel and cleaning the output before you commit.Fix all of Yori's comments.Laura KetzerLaura Ketzerhttps://gitlab.aip.de/lketzer/platypos/-/issues/4CLEANING: remove *.pyc from the repository2020-06-26T08:35:06ZYori Fourniery.fournier@aip.deCLEANING: remove *.pyc from the repositoryYou should add all `*.pyc` in `.gitignore` for two reasons:
1. they will be committed each time you run the code, that will make your repo quite big.
2. It pollutes the diffs of your commits, it become more difficult to see what you have...You should add all `*.pyc` in `.gitignore` for two reasons:
1. they will be committed each time you run the code, that will make your repo quite big.
2. It pollutes the diffs of your commits, it become more difficult to see what you have changed from one commit to another.
Alike with the jupyter notebooks, before commit `stop kernel and clean output`. When you let the outputs inside you can not see the changes in the web interface.Fix all of Yori's comments.Laura KetzerLaura Ketzerhttps://gitlab.aip.de/lketzer/platypos/-/issues/5IDEA: usage of your package2020-06-26T08:35:05ZYori Fourniery.fournier@aip.deIDEA: usage of your packageIn this jupyter notebook [V1298Tau_Paper_Calculations_and_Plots.ipynb](https://gitlab.aip.de/lketzer/platypos/-/blob/master/example_V1298Tau/V1298Tau_Paper_Calculations_and_Plots.ipynb) I could find a few technical things you may improve...In this jupyter notebook [V1298Tau_Paper_Calculations_and_Plots.ipynb](https://gitlab.aip.de/lketzer/platypos/-/blob/master/example_V1298Tau/V1298Tau_Paper_Calculations_and_Plots.ipynb) I could find a few technical things you may improve:
* Do not import your package via changing the path!
* You can easily make the fonts to be latex (it looks much better in a paper)
* I would suggest not to use `plt.subplot` to create `Figure` and `Axes` it used to have a memory leak.
* a one column plot is 8 inch wide not 10. You may also want to fix the dpi to 300 for a better export.
Feel free to ask in the comment if you are interested about another way of doing these stuff.Fix all of Yori's comments.Laura KetzerLaura Ketzerhttps://gitlab.aip.de/lketzer/platypos/-/issues/6Cleanning: wierd `while`construction2020-06-26T08:35:06ZYori Fourniery.fournier@aip.deCleanning: wierd `while`constructionWhy this [line](https://gitlab.aip.de/lketzer/platypos/-/blob/9a7763654fd137a49e1820de142cb1ac30ef9c87/platypos_package/Planet_class_LoFo14.py#L65)?
I guess this is sufficient (rmk: break is commented)
```python
try:
...Why this [line](https://gitlab.aip.de/lketzer/platypos/-/blob/9a7763654fd137a49e1820de142cb1ac30ef9c87/platypos_package/Planet_class_LoFo14.py#L65)?
I guess this is sufficient (rmk: break is commented)
```python
try:
# check if envelope mass fraction is specified, then Case 1
planet_dict["fenv"]
self.planet_info = "Case 1 - artificial planet"\
+ " (with M_core & f_env)" # use formatting for that (str.format())
# adding a blank line here helps for reading
# Case 1: artificial planet with fenv & M_core given, need to
# calculate the total mass and radius
self.fenv = planet_dict["fenv"]
self.core_mass = planet_dict["core_mass"]
self.Calculate_core_radius() # not PEP8
self.Calculate_planet_mass() # not PEP8
self.Calculate_planet_radius() # not PEP8
# break
# adding a blank line here help for reading
except KeyError:
...
```
1. PEP8 suggest that method start with lower case.
2. instead of adding strings better to use formatting:
```python
self.planete_info = "Case {case_index:d} - {description} ({details})"
self.planete_info.format(case_index=1,
description="artificial planete",
details="with M_core & f_env")
```Fix all of Yori's comments.Laura KetzerLaura Ketzerhttps://gitlab.aip.de/lketzer/platypos/-/issues/7CLEANING: some PEP8 tips2020-06-26T08:35:06ZYori Fourniery.fournier@aip.deCLEANING: some PEP8 tipsExample from [there](https://gitlab.aip.de/lketzer/platypos/-/blob/master/platypos_package/Planet_class_LoFo14.py#L152)
```python
def Calculate_R_env(self):
""" Check Planet_models_LoFo14.py for details on input and
...Example from [there](https://gitlab.aip.de/lketzer/platypos/-/blob/master/platypos_package/Planet_class_LoFo14.py#L152)
```python
def Calculate_R_env(self):
""" Check Planet_models_LoFo14.py for details on input and
output parameters;
R_env ~ t**0.18 for *enhanced opacities*;
R_env ~ t**0.11 for *solar metallicities*
"""
age_exponent = {"solarZ": -0.11, "enhZ": -0.18}
R_env = 2.06 * self.mass**(-0.21) * (self.fenv/5)**0.59 * \
self.flux**0.044 * \
((self.age/1e3)/5)**(age_exponent[self.metallicity])
return R_env # in units of R_earth
```
## Great stuff
### Documentation
As I told you the doc is very good already. So continue it will help. as a tip did you know that you could do:
```python
import platypos
help(platypos.planet_LoFo14.Calculate_R_env)
```
it will print:
```bash
Check Planet_models_LoFo14.py for details on input and
output parameters;
R_env ~ t**0.18 for *enhanced opacities*;
R_env ~ t**0.11 for *solar metallicities*
```
### variable names
`age_exponent` is a good variable name it is clear.
### Comments
You have a good balance of comments, keep it that way.
## Issues
### Documentation
You may have a look at the `Sphynx` package and `Numpy documentation style`.
It will reward you with all the time you spend on writing docs.
So far they are good, but if you follow the `Numpy` standards you
will really feel like a goddess when `Sphynx` can produce a website
with the entire code documentation in 1 command line!!!
### Variable names
* `R_env` is not clear. radius of the environment? radius of the envelop I guess.
You can keep the variable name like that but at least mention it in the doc.
* `fenv` is a bad name, you should follow one rule: `f_env` or `F_env`
### Comments
Stating units in comments is very good (because comments are for the developer and it is always nice to see the unit straight away, while developing). But the user needs it too! put it in the docs.
### PEP8
```python
R_env = 2.06 * self.mass**(-0.21) * (self.fenv/5)**0.59 * \
self.flux**0.044 * \
((self.age/1e3)/5)**(age_exponent[self.metallicity])
```
should be (PEP8):
```python
R_env = 2.06 * self.mass ** -0.21 * ( self.f_env / 5. ) ** 0.59 \
* self.flux ** 0.044 \
* ( self.age / 1.e3 / 5.) ** ( age_exponent[self.metallicity] )
```
I personally would write:
```python
R_env = 2.06 * self.mass**(-0.21) * ( self.f_env / 5. )**(0.59) \
* self.flux**(0.044) \
* ( self.age / 1.e3 / 5.)**( age_exponent[self.metallicity] )
```Fix all of Yori's comments.Laura KetzerLaura Ketzerhttps://gitlab.aip.de/lketzer/platypos/-/issues/8CLEANING: dictionary in constructor are usually not a good idea2020-06-26T08:35:06ZYori Fourniery.fournier@aip.deCLEANING: dictionary in constructor are usually not a good idea[That](https://gitlab.aip.de/lketzer/platypos/-/blob/master/platypos_package/Planet_class_LoFo14.py#L44) is not what you are suppose to do.
What do you want to achieve with a dictionary? Why not using keywords? If it is to avoid writing...[That](https://gitlab.aip.de/lketzer/platypos/-/blob/master/platypos_package/Planet_class_LoFo14.py#L44) is not what you are suppose to do.
What do you want to achieve with a dictionary? Why not using keywords? If it is to avoid writing all the arguments when calling the function be aware that there are method for that:
```python
class MySuperClass(object):
'''This is a dummy class just for you
Attributes:
-----------
dummy_member: float
That is the main part of this class.
'''
def __init__(self, dummy_member, time_ten=False, multiplicator=None, power_of_two=True, *args, **kwargs):
'''Initialize a new instance (=object) of MySuperClass
Parameters:
-----------
dummy_member : float
value of the dummy_member
time_ten : bool, optional, default=False
if set to True dummy_member value will be multiplied by 10
multiplicator : float, optional, default=None
if given multiplies the dummy_member value.
power_of_two : bool, optional, default=True
if True the given dummy_member value will be squared.
'''
self.dummy_member = dummy_member
if time_ten:
self.dummy_member = 10. * self.dummy_member
if multiplicator is not None:
self.dummy_member = multiplicator * self.dummy_member
if power_of_two:
self.dummy_member = self.dummy_member * self.dummy_member
```
This can then be called like:
```python
planete_keywords = {'time_ten': True,
'multiplicator': 25.,
'power_of_two': True }
planete = MySuperClass(120., **planete_keywords)
```
the `*` and `**` operators transform a `tuple` and a `dict` into a list of `parameters` and `keywords` respectively.
```python
def function_with_params(a, b, c):
...
return(d)
```
can be called:
```python
function_with_params(1., 2., 3.)
```
or in the same way
```python
params = (1., 2., 3.)
function_with_params(*params)
```
the same for keywords:
```python
def function_with_keywords(a, b, kwa=None, kwb=True):
...
return(w)
```
can be called like
```python
function_with_keywords(2., 3., kwa=12., kwb=False)
```
or that way:
```python
params = (2., 3.)
keywords = {'kwa': 12.,
'kwb': False}
function_with_keywords(*params, **keywords)
```Fix all of Yori's comments.Laura KetzerLaura Ketzerhttps://gitlab.aip.de/lketzer/platypos/-/issues/10IDEA Cleaning: I won't do that but that is ok.2020-06-26T08:35:06ZYori Fourniery.fournier@aip.deIDEA Cleaning: I won't do that but that is ok.In this function you define a function in a function that is ok, but in this case not necessary.
see [https://gitlab.aip.de/lketzer/platypos/-/blob/master/platypos_package/Planet_class_LoFo14.py#L133](https://gitlab.aip.de/lketzer/platy...In this function you define a function in a function that is ok, but in this case not necessary.
see [https://gitlab.aip.de/lketzer/platypos/-/blob/master/platypos_package/Planet_class_LoFo14.py#L133](https://gitlab.aip.de/lketzer/platypos/-/blob/master/platypos_package/Planet_class_LoFo14.py#L133)
```python
def Solve_for_fenv(self):
""" For known core and planet radius, core mass, age and flux,
solve for envelope mass fraction."""
if self.radius == self.core_radius:
self.fenv = 0.0
else:
def Calculate_fenv(fenv):
age_exponent = {"solarZ": -0.11, "enhZ": -0.18}
return -self.radius + self.core_radius + (2.06\
* (self.core_mass/(1-(fenv/100)))**(-0.21)\
* (fenv/5)**0.59 * (self.flux)**0.044\
* ((self.age/1e3)/5)**(age_exponent[self.metallicity]))
f_guess = 0.1
fenv = optimize.fsolve(Calculate_fenv, x0=f_guess)[0]
self.fenv = fenv
# if fenv1 != fenv:
# print("Sth went wrong in solving for\
# the envelope mass fraction! Check!")
```
defining a function inside a function is totally fine in python. Even a class in a class...
However, it might be a bit difficult to debug. Because the function exists only in the namespace of the other function.
So even some time it is necessary to define a function into another function (a decorator for instance) here it is not.
And what is not necessary should be avoided.
```python
@staticmethod
def calculate_fenv(fenv):
age_exponent = {"solarZ": -0.11, "enhZ": -0.18}
return -1. * self.radius + self.core_radius \
+ (2.06 * (self.core_mass / (1 - (fenv / 100) ) )**(-0.21) \
* (fenv / 5)**0.59 * (self.flux)**0.044 \
* ( (self.age / 1.e3) / 5)**(age_exponent[self.metallicity]))
def Solve_for_fenv(self):
""" For known core and planet radius, core mass, age and flux,
solve for envelope mass fraction."""
if self.radius == self.core_radius:
self.fenv = 0.0
else:
f_guess = 0.1
fenv = optimize.fsolve(self.calculate_fenv, x0=f_guess)[0]
self.fenv = fenv
# if fenv1 != fenv:
# print("Sth went wrong in solving for\
# the envelope mass fraction! Check!")
```
The funny part here is that the `@staticmethod` decorator (decorator?? read the docs that is one of the most crazy part of python) actually defines `calculate_fenv` inside another method. X) but it does properly and debugging won't be an issue.Fix all of Yori's comments.Laura KetzerLaura Ketzerhttps://gitlab.aip.de/lketzer/platypos/-/issues/14Cleaning: read_results, why do you use DataFrame if you don't?2020-06-26T08:35:07ZYori Fourniery.fournier@aip.deCleaning: read_results, why do you use DataFrame if you don't?There: [https://gitlab.aip.de/lketzer/platypos/-/blob/master/platypos_package/Planet_class_LoFo14.py#L297](https://gitlab.aip.de/lketzer/platypos/-/blob/master/platypos_package/Planet_class_LoFo14.py#L297)
Using `DataFrame` just for rea...There: [https://gitlab.aip.de/lketzer/platypos/-/blob/master/platypos_package/Planet_class_LoFo14.py#L297](https://gitlab.aip.de/lketzer/platypos/-/blob/master/platypos_package/Planet_class_LoFo14.py#L297)
Using `DataFrame` just for reading is a bit under utilizing the potential of `pandas`. There are plenty of other modules that can read .csv files without creating the entire structure of a `DataFrame` behind it.
So I would suggest either to use the `DataFrame` further in the program of to use a simpler library to read the .cvs file.
But using the `DataFrame` will provide you great features in the rest of the program. `read_results` could just return `df`.Fix all of Yori's comments.Laura KetzerLaura Ketzer