search

pynbody / genetIC

The GM initial conditions generator
GNU General Public License v3.0
21 stars 8 forks source link

Const refact #8

Closed Martin-Rey closed 7 years ago

Martin-Rey commented 7 years ago

Hi Andrew,

I am pushing this branch mainly if you want to take a look at the changes. The constraints namespace is meant to disappear completely but is kept for easy copy pasting at the moment. i will investigate storing the convectors when I find the time

Feel free to take a look (or not if you don't have the time). No need to accept the branch yet as it is definitely not its final state.

Martin

Martin-Rey commented 7 years ago

All compatibility problems seem to be solved. I had introduced a dump_modifications previously which I used in your new test.

Attention, modify is the new keyword command instead of constrain. Some underscores in command names have been changed for consistency.

Martin-Rey commented 7 years ago

Ready to merge on my side. As I said, the modification namespace has plenty of work left to be done but feel free to comment anything.

apontzen commented 7 years ago

I'd really like to resolve the 'dump' double meaning. I don't think we should use it for "cancel"... happy also to have it not mean "save" either but at the moment it's used to mean both in different places and that's truly asking for trouble!

fix_modifications should maybe be somehing more obvious like apply_modifications (I know fix was my choice but it looks vague on reflection)

Other than that, good to go...

Martin-Rey commented 7 years ago

Ok. I'll replace it with "clear" where it has the meaning of releasing memory and keep the appellation when it has the meaning of output to a file, which seems standard.

https://stackoverflow.com/questions/8226477/what-is-a-dump-both-software-wise-and-hardware-wise

Martin-Rey commented 7 years ago

Note that I have modified the CMakeFile to try to make it as blind as possible to local configurations. If you get too much compatibility problems when building, I'll revert to the original and keep my copy locally.

apontzen commented 7 years ago

Thanks for the alert about CMakeLists.txt -- the main problem it's causing me is your treatment of warnings. Could you remove -Werror -Wfatal-errors?

In general, I approve of your desire to squash warnings, but different compilers issue different warnings so the -Werror -Wfatal-errors makes it compile with some compilers and not others (specifically I had problems with macports clang 4.0).