search

wright-group / WrightSim

A simulation package for multidimensional spectroscopy.
MIT License
4 stars 0 forks source link

Comments and docstrings #20

Closed ksunden closed 6 years ago

ksunden commented 6 years ago

Just some initial comments and docstrings and such, wanted someone else to read to ensure that I say what I mean to say, and that it makes sense.

Thanks

ksunden commented 6 years ago

We may have some style inconsistencies, particularly in the C method description block comments. I'm not worrying about that too much at this time, but it may be worth deciding on a styleguide to follow in the future.

ksunden commented 6 years ago

I have added the (optional) marker for now to the hamiltonian __init__. Admittedly, I didn't add it before partly because all of them are optional, partly because I think we may wish to do a bit better abstraction and have a base either without optional parameters, or at least some required fields. What makes it a little tricky is if we wish to change the ordering...

Having a base hamiltonian that is not any particular useful hamiltonian may be beneficial, simplifying some code, and allowing for easier reparameterization (e.g. w_central and coupling values, nonsensical for many potential hamiltonians, but useful for TRIVE). We then may wish to reorder the parameters from their current order. We may also wish to make some of the parameters not changeable via the init method, e.g. subclasses define the state labels, and do not necessarily expose those to the user in the init method.

What we have now is great, it works, but I think we may wish to have an abstracted version of this. If done well, we may be able to have a single C struct for the CUDA implementation of all Hamiltonians (or at least most), reducing the need to program that and the transfer method for every hamiltonian.

ksunden commented 6 years ago

ping @untzag

I plan on merging this tomorrow, however, if there are still comments, please let me know, I will address them as soon as I am able