animikhroy / rk_toolkit_pipeline_diagrams

Master-repository for all code related to "A Novel Approach to Topological Graph Theory with R-K Diagrams and Gravitational Wave Analysis"
https://arxiv.org/abs/2201.06923
2 stars 0 forks source link

moved files around to begin pruning process. #1

Closed andorsk closed 2 years ago

andorsk commented 2 years ago

Status

Ready for Review

Problem

Files are all over the place and it makes it hard for someone to know what to look for and what needs work.

Description

Pruning and cleaning everything up, imposing structure on what I think eventually a final repo will be. The structure is described below ( subject to change ) :

notebooks
  \_store_sales.ipnyb
  \_ligo.ipynb
rk_toolkit
publications
   \_arxiv
  \_peer_reviewX
rk-visualizer

Scope

This PR does not have:

  1. Additional Comments
  2. Actual Refactoring of Code to work with the documentation
  3. Is solely scoped as a restructure PR.

It is focused on the initial restructuring of the folders and how everything should be moved together in the final repo.

animikhroy commented 2 years ago

Here are my re-structuring comments before merging this PR:

1) Structure of the final repo: Since Dr. Ravi is not available today, I came to the following conclusion after an initial discussion with members of IUCAA who have submitted to PR, I believe the structure of the final repo should represent the sequence in the paper while allowing for future development work and further additions. Hence this is the revised structure I would propose:

1st : rk_toolkit (with corresponding readme and updated instructions) 2nd: notebooks: \rk_general_applications \ _store_sales.ipnyb (to make it clear to respective reviewers and allow for further assimilation of general applications in that sub folder in future) \rk_gw_mma \ rk_ligo.ipnyb (to allow a clear distinction for reviewers to find all the relevant notebooks on applications of the r-k pupekine in gw (gravitational wave astronomy) & mma (Multimessenger Astronomy) in one place and for future additions + developments 3rd: rk-visualizer (this should come 3rd as this will be a part of the code review) 4th : publications: _arxiv _physical_reviewX (note PR stands for Physical Review and not peer_review) _physical_reviewD (note PR stands for Physical Review and not peer_review) This 4th part will be entirely owned and controlled by me (animikhroy) henceforth with pull requests sent to others whenever any specific review/suggestion is required. I will maintain similar copies of the latex files on both with modified formatting and specific changes relevant to that particular submission.

So once the above changes are incorporated the restructured repo should look like this:

01_rk_toolkit 02_notebooks \ _rk_general_applications \ _store_sales.ipnyb \ _rk_gw_mma \ _rk_ligo.ipnyb 03_rk-visualizer 04_publications \ _arxiv \ _physical_reviewX \ _physical_reviewD LICENSE (creative commons) ReadMe.md

2) I have some additional comments on the TODO section for the prioritization of tasks and responsibilities as mentioned on a separate PR for your review.

andorsk commented 2 years ago

@animikhroy where do common source files live across the two notebooks?

andorsk commented 2 years ago

Updated with new structure.

andorsk commented 2 years ago

@animikhroy notice I also added the license CC to the repo

animikhroy commented 2 years ago

@andorsk Thanks a lot for the revised structure and all the corresponding updates. Comments:

1) I see you have moved all the common source files under the rk_toolkit folder and that is what I would have suggested as well. So I think that is perfectly fine.

2) I like the new structure and it overall looks good to me however I shall do a short review with an IUCAA postdoc today who has recently published on PR-D&X and get back to you with any necessary inputs if required.

3) I did notice you have included the CC license to the repo and thanks for making sure!

andorsk commented 2 years ago

@animikhroy There are common files (not in the rk toolkit ) that will exist. Those aren't currently in the repo as organized right now. I'm guessing 02notebooks \ common or something but please let me know.

animikhroy commented 2 years ago

@andorsk sorry for the misunderstanding and thanks for the clarification. I think in that case the notebooks folder should have a subfolder called \00_common_files which could be used across all use cases. Hence the directory could be restructured as follows :

02_notebooks \ _00_common_files \ _rk_general_applications \ _store_sales.ipnyb \ _rk_gw_mma \ _rk_ligo.ipnyb

Does that address the issue or am I still missing something?

andorsk commented 2 years ago

nope. that addresses it. Thanks.

animikhroy commented 2 years ago

@andorsk so as mentioned earlier I did have a detailed feedback session with one assistant professor and the a senior scientific officer working with the LIGO collaboration who have published on PR-D/X and they had the following pointers and impressions:

1) Overall very happy and impressed to go through our initial set up and confirm we are on the right direction to meet all the necessary requirements of PR-D/X.

Some important feedback with reference to the notion page items below:

https://dot-terrier-969.notion.site/686f90e5615445c5bdd7753d32e5028f?v=1256c9f6528142a4aa367ef1d1d90b23

2) TODO ITEM 9: Add docstrings to methods (already somewhat there). Serve sphinx docs : please follow PyCBC documentation structure and guidelines : https://pycbc.org/pycbc/latest/html/devs.html they also use sphinx and we are on the right track but we need to follow their basic format for PR-D/X submissions. Here are the relevant links in that regard for you and Dr. Ravi and anyone else collaborating: (a) https://pycbc.org/pycbc/latest/html/documentation.html (b) https://www.sphinx-doc.org/en/master/tutorial/index.html

3) TODO ITEM 10: Write more unit tests. See tests folder in rk_toolkit for samples : Please provide all unit tests as jupyter notebooks once they have been finalized for submission as this is another requirement criteria of PR-D/X reviewers.

4) Add more doc strings to rk-visualizer.py as a part of TODO ITEM 9 in the same format as suggested.

5) TODO ITEM 8: General code clean up and refactoring: The actual final code needs to be updated on \ _store_sales.ipnyb and \ _rk_ligo.ipnyb for one additional round of review (@andorsk I believe you have already planned to do this as there are 33 lines of some place-holder sample code in those 2 main sections right now)

6) TODO ITEM 8: General code clean up and refactoring: There needs to be a separate labelled file containing code exclusive to the "R-K distance" function residing within the R-K toolkit (if not then in the common_files in the notebooks) as this is a core part of our unique IP in the paper which will be evaluated and applied to future use cases. There might be a unit test that needs to be added for this section as well. This will be important for the acceptance criteria of our novel approach just like the other unique aspects of the paper are clearly labelled and represented in the code for reviewers to evaluate.

andorsk commented 2 years ago

@animikhroy good notes. Make sure they get relayed to the relevant people. As far as what I've told I'll be helping with is:

  1. Some refactoring to make it easier for Dr. Ravi to get in and do the items 8-12.
  2. 1-4 Action items ( which will take time and as I already mentioned, are taking longer than expected ).

I see 9 items you've tagged me on, which I don't believe is correct. For example, I am not owning 8-12. We discussed Dr. Ravi will help with that and he/you should lead that. I'll help give guidance where necessary I'm noting here I'm not going to take responsibility for them so make sure you manage those responsibilities.

Finally, as I've mentioned 1 billion times, all of this is contingent on availability and I have not committed to aside of the fact that I would try to get this done and understand the importance these deadlines present to you.

I will merge this PR, and start moving the existing stuff over in a way that makes sense.

Thanks!

andorsk commented 2 years ago

The refactoring work is taking place here: https://github.com/animikhroy/rk_toolkit_pipeline_diagrams/pull/3