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

Rewrite branch needs to consolidate code #30

Closed
sweverett opened this issue Feb 11, 2020 · 6 comments
Closed

Rewrite branch needs to consolidate code #30

sweverett opened this issue Feb 11, 2020 · 6 comments

Comments

@sweverett
Copy link
Owner

Right now the rewrite branch has the main code split into two copies - clustr.py and newnewclustr.py. That's no good! We need to consolidate into a single file that we all work off, namely clustr.py (though we can keep some of the new implementations from newnewclustr.py).

Taking a quick look at the two copies, I suggest that we use keep the following from each file:

clustr.py:

  • Config
  • The additional argparse arguments in the ArgumentParser class, but move them into the correct format in newnewclustr.py
  • E(z) function (this is a cosmological function)
  • The get_data() function in the Catalog class (which we will co-opt into the new Data() constructor

newnewclustr.py:

  • The new parser structure (at the top of the file)
  • main() function, as it's using the new OO design
  • Catalog, as we're restructuring it in the new design

Remember that we're not going to 'lose' anything by consolidating - the main code is still in the master branch! We can still reuse things from the main code if we find them helpful, but we're also not forced to use it.

@sweverett
Copy link
Owner Author

#29

@sweverett
Copy link
Owner Author

One thing that I noticed - the argument parsing was moved from the top of the file and into a new class also called ArgumentParser() in aa35d11. This will fail for a couple independent reasons, and we actually want it at the top of the file and outside of the class. We should revert this change.

@sweverett
Copy link
Owner Author

I also noticed some bugs in the indentation and function syntax throughout - we should make sure that the copied over code is being done correctly and put at the right indentation levels. E(z) is an example of this.

@sweverett
Copy link
Owner Author

Here is how we can solve some of our versioning history and credit problems:

(1) Go to your local repo and commit any local changes that you have
(2) Grab other people's changes with git pull origin rewrite
(2a) deal with any merge conflicts that you might have
(3) Rename the code we will no longer be using: git mv clustr.py old_clustr.py
(4) Rename the "official" code to the right name: git mv newnewclustr.py clustr.py
(5) Commit the change
(6) git push origin rewrite
(7) Everyone else needs to commit local changes and then git pull origin rewrite to get the filename changes

@sweverett
Copy link
Owner Author

OR, it would be simpler just to use clustr.py as the "official" version and incorporate the changes in newnewclustr.py you want to keep into clustr.py

jjobel added a commit that referenced this issue Feb 27, 2020
@sweverett
Copy link
Owner Author

I believe this was completed a while ago - closing for now.

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

No branches or pull requests

3 participants