add classmethod from_Table

[nstarman]

Signed-off-by: Nathaniel Starkman nstarkman@protonmail.com

[webbjj]

Everything looks good, except I think the load from table routine you wrote should be in load.py instead of cluster.py. Also what does typing do? I would other want to either change units and origin in your load script to what I normally use in clustertools or switch everything over.

[nstarman]

I've added it to load.py. However, the benefit of having a class-method loader is that any child classes of StarCluster will inherit the load method. ie

class NewClass(StarCluster): pass
cluster = NewClass.from_Table(*stuff)

Will produce a NewClass object using the same method. To accommodate this, I added a cls argument to the load.py method and retained the class-method in StarCluster, but it just calls the load.py method. I've also added "table" as a flag option for ctype in the function load_cluster.

As for the module typing, it is built into python to provide type hinting for faster run speed on compilation. A fully (and correctly) type hinted code can be compiled into static-typed code rather than dynamic typed code, and is orders of magnitude faster. There is no need to change the units and origin arguments, this just augments how python compiles the code.

[jobovy]

Note that using typing sets the minimum Python version requirement to 3.5, which you may want to mention in the docs then. FWIW, I don't think that's a problem as nobody should really be on 3.4 or earlier anymore...

[webbjj]

Thanks for the heads up, I was unaware of this. I ended up dropping the typing for now anyway since it wouldn't be consistent with the rest of the code and I would like to use it a bit first to understand it better.

Do you get a notification every time there is a push? Or a PR? I have been pushing a lot lately to update readthedocs, so sorry if that has flooded your inbox.

Jeremy

On Fri, Jul 17, 2020 at 12:11 PM Jo Bovy notifications@github.com wrote:

Note that using typing sets the minimum Python version requirement to 3.5, which you may want to mention in the docs then. FWIW, I don't think that's a problem as nobody should really be on 3.4 or earlier anymore...

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/webbjj/clustertools/pull/1#issuecomment-660194833, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB7AIYDBHSLG4QZ6EVQV3ILR4BZ27ANCNFSM4O3QG7JA .

[jobovy]

Oh, I didn't see that you removed it (BTW, you can just push to @nstarman's branch while the PR is open if you want to change things before merging).

I get emails for PRs because I 'watch', pushes don't trigger emails, don't worry!