marwahaha commented 2 years ago

fixes #88 As projects get larger, I think it's worth breaking out the files into folders. Some teams (especially backend teams) like to split out into one file per class or struct. I tried to find a medium between the current state of the project and that.

Adding folders may discourage a new user, but I think this approach is better for being able to make small adjustments without having several hundred lines' context in your head. For example, after I did this, I tried merging IonInstance and Ion, and then put it back. It was a lot easier this way.

Let me know what you think.

Also: Is our convention for phrases:

with underscores, like ion_configuration
with capital letters, like IonConfiguration
with no spacing, like ionconfiguration

I think we use different ones in different places. I'd like to be consistent.

This is the directory now.

(base) kunal@kunal-hp:~/.julia/dev/IonSim$ tree .
.
├── LICENSE.md
├── Manifest.toml
├── Project.toml
├── README.md
├── scripts
│   └── format.sh
├── src
│   ├── analytic_functions.jl
│   ├── constants.jl
│   ├── hamiltonian
│   │   ├── hamiltonian.jl
│   │   ├── _include_hamiltonian.jl
│   │   ├── internal.jl
│   │   └── setup_hamiltonian.jl
│   ├── helpers.jl
│   ├── ion
│   │   ├── _include_ion.jl
│   │   ├── ioninstance.jl
│   │   ├── ion.jl
│   │   ├── ionproperties.jl
│   │   ├── levels.jl
│   │   ├── quantumnumbers.jl
│   │   ├── species
│   │   │   ├── be9.jl
│   │   │   ├── ca40.jl
│   │   │   ├── _include_species.jl
│   │   │   ├── mg25.jl
│   │   │   └── yb171.jl
│   │   ├── stark_shift.jl
│   │   ├── transitions.jl
│   │   └── zeeman_shift.jl
│   ├── ion_configurations.jl
│   ├── ion_operators.jl
│   ├── IonSim.jl
│   ├── lasers.jl
│   ├── time_evolution.jl
│   ├── trap
│   │   ├── bfield.jl
│   │   ├── efield.jl
│   │   ├── _include_trap.jl
│   │   └── trap.jl
│   └── vibrational
│       ├── _include_vibrational.jl
│       ├── vibrational_mode.jl
│       └── vibrational_operators.jl
└── test
    ├── runtests.jl
    ├── runtests.sh
    ├── test_analytic_functions.jl
    ├── test_constants.jl
    ├── test_dynamics.jl
    ├── test_hamiltonians.jl
    ├── test_ion_configurations.jl
    ├── test_ions.jl
    ├── test_lasers.jl
    ├── test_operators.jl
    ├── test_time_evolution.jl
    ├── test_traps.jl
    └── test_vibrational_modes.jl

8 directories, 51 files

codecov[bot] commented 2 years ago

Codecov Report

Merging #87 (cae5c00) into master (e62be49) will not change coverage. The diff coverage is 90.7%.

@@          Coverage Diff           @@
##           master     #87   +/-   ##
======================================
  Coverage    92.3%   92.3%           
======================================
  Files          15      28   +13     
  Lines        1165    1165           
======================================
  Hits         1075    1075           
  Misses         90      90

Impacted Files	Coverage Δ
src/IonSim.jl	`100.0% <ø> (ø)`
src/ion/species/be9.jl	`100.0% <ø> (ø)`
src/ion/species/ca40.jl	`100.0% <ø> (ø)`
src/ion/species/mg25.jl	`100.0% <ø> (ø)`
src/ion/species/yb171.jl	`33.3% <ø> (ø)`
src/ion_configurations/ion_configurations.jl	`100.0% <ø> (ø)`
src/lasers/lasers.jl	`97.8% <ø> (ø)`
src/mathematical_functions/analytic_functions.jl	`100.0% <ø> (ø)`
src/vibrational/vibrational_mode.jl	`100.0% <ø> (ø)`
src/ion/ion_instance.jl	`64.7% <64.7%> (ø)`
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

jbroz11 commented 2 years ago

I really like this structure.
I moved some things around since the organization of efield/ bfield didn't make physical sense.
Some things were broken like transition_frequency was being exported but the function name had been changed to transitionfrequency (which was not being exported). This was pretty concerning since:
- why wasn't the unit testing catching this?
- edit: the answer here seems to be that there was a duplicate definition in the ion/transitions.jl file
- what else has been broken by recent modifications that unit testing hasn't caught.
@neil-glikin , @marwahaha Note: function doc strings need to begin with indented signature in order to be parsed appropriately see here.
@neil-glikin can you combine and restructure the the code in convenience_functions/ion_queries.jl and ion/transitions.jl (the things that have to with eg. transitionfrequency and matrixelement which has methods defined in both files currently) as well as ion/zeeman_shift.jl and convenience_funcitons/ion_queries.jl. This might end up eliminating the need for ion_queries.jl...
As far as conventions for phrases, what I was crudely using was camel case for types, underscores for file names and no underscores for function names. But that hasn't been consistently maintained from what I've seen(at least wrt. to function names). I'm also ambivalent on the convention that we use.

marwahaha commented 2 years ago

This sounds fine to me. @jbroz11 can you add a line in the readme about the docstring indents? I tried and failed to find an automated solution to enforce it.

neil-glikin commented 2 years ago

I was hoping to restructure this as such that I consolidate the definitions of functions such as matrix_element, transitionfrequency, and zeeman_shift which had previously been in separate files together. However the "other" methods of these functions all involve a Trap, which is not defined until traps.jl, while these functions all relate to Ions.

This puts me in a bit of a catch-22: I can't tell IonSim to include the trap folder before the ion folder since Traps use Ions, but I also can't tell IonSim to include the ion folder before the trap folder since I'm now trying to put ion-related methods involving traps into the ion folder.

Does this mean that structuring this way simply can't be done? Or is there a better way? I could push a commit with the new file structure but in which IonSim does not compile in order to show you what I'm thinking, if someone thinks that would be reasonable and useful.

marwahaha commented 2 years ago

Instead of putting that file in the _include_... file within ion folder, we could just import that file directly after trap. Be sure to leave a note in the code if you do though.

Why do the methods belong together?

jbroz11 commented 2 years ago

So, one thing that makes me skittish here is that it seems like moving files around will erase the git history for those files. Of course, the history of the whole project is saved so (at least in our case) it's not so difficult to go back through the old commits and figure out how things developed. But merging this pull request would effectively hide the authorship of the moved files from all but the most careful investigation.

For this reason, I think we should put a pin in this for now. Thoughts @marwahaha @neil-glikin?

marwahaha commented 2 years ago

I think you're right, debugging "who made what method" will be harder to immediately see. On the other hand, there's never a right time to make a change like this. (It will only hurt more later.)

I think we should agree on a structure now, and my vote is to move forward.

jbroz11 commented 2 years ago

Let's discuss it more next meeting. I'm going to close the pr for now.

neil-glikin commented 2 years ago

Instead of putting that file in the _include_... file within ion folder, we could just import that file directly after trap. Be sure to leave a note in the code if you do though.

Why do the methods belong together?

Not sure they do belong together, just thought it would be more clear if one could find all methods of a function together. But this is probably a broader question of code structure philosophy that we can decide on.

neil-glikin commented 2 years ago

I know we've closed the PR for now but wanted to add my own suggested changes for the record. They're pushed to ion-reorg. (Made it work with a hacky way of ordering include statements in IonSim.jl, should be easy to clean up later.)

I moved several things from convenience_functions/ion_queries.jl to other places.
helpers.jl and analytic_functions.jl have been moved to the folder mathematical_functions
constants.jl has been split into mathematical_constants.jl and physical_constants.jl
levels.jl now includes functions to get the main properties of levels/sublevels: energy and quantumnumbers. This makes it a little longer; I'm open to moving energy and quantumnumbers back elsewhere/to their own files.
ion_queries.jl became pretty sparse and I didn't see a meaningful difference between it and set_trap_parameters.jl, so I combined them into convenience_functions.jl, but I'm open to undoing it.

marwahaha commented 2 years ago

It doesn't really make sense to me to close the PR. I think we should keep experimenting and use it as discussion for our next meeting.

marwahaha commented 1 year ago

I'm reopening this because I think I've addressed Joe's main issue -- attributing credit properly. All of my commits have been replaced with Joe's name and email.

HaeffnerLab / IonSim.jl

Reorganize code #87

Codecov Report