Comments and docstrings #20
Conversation
|
We may have some style inconsistencies, particularly in the C method description block comments. |
| Shape T x N x N array with the full Hamiltonian at each time step. | ||
| N is the number of states in the Density vector. | ||
| """ | ||
| #TODO: Just put the body of this method in here, rather than calling _gen_matrix |
There was a problem hiding this comment.
I'm going to hold off for this PR, because I want this to be as much as possible limited to non-code, i.e. comments and docstrings. This is one of the first things to be done when I touch the code again though. This is issue #26.
WrightSim/hamiltonian/default.py
Outdated
| except populations, which default to np.inf. | ||
| Default is 50. fs (scalar, then brodcast as above. | ||
| mu : 1-D array <Complex> | ||
| The magnetic suceptabilities used either in computing state densities or output intensities |
There was a problem hiding this comment.
I don't know why you use the word "magnetic" (you go on to use it several times)
how about just "susceptibilities"? or "dipole moment"?
unless I'm mistaken (which I often am) these are not magnetic susceptabilities
There was a problem hiding this comment.
You are correct, my tired brain was mixing up susceptibilities and dipole moments, my mistake, thanks for catching it
WrightSim/hamiltonian/default.py
Outdated
| The lifetime of the states in femptoseconds. | ||
| If tau is scalar, the same lifetime is used for all transitions, | ||
| except populations, which default to np.inf. | ||
| Default is 50. fs (scalar, then brodcast as above. |
There was a problem hiding this comment.
(scalar, then brodcast as above
don't know what this means? unintentional?
There was a problem hiding this comment.
I'm reading this again, and agree it's unclear, rather than try to explain what I meant here, I'm just going to have another go at writing it from scratch, hopefully that's clearer
WrightSim/hamiltonian/default.py
Outdated
| mu : 1-D array <Complex> | ||
| The magnetic suceptabilities used either in computing state densities or output intensities | ||
| Array like structures are converted to the proper data type. | ||
| Order mattersd, and meaning is dependent on the individual Hamiltonian. |
WrightSim/hamiltonian/default.py
Outdated
| The initial density vector, length defines N. | ||
| Default is 1. in the ground state (index 0), and 0. everywhere else. | ||
| tau : scalar or 1-D array | ||
| The lifetime of the states in femptoseconds. |
There was a problem hiding this comment.
there are many rates to consider, so I worry that users will be confused...
perhaps be explicit:
for coherences, tau is the pure dephasing time
for populations, tau is the population lifetime
I always get confused... are we sure tau is a time here? or is it actually a rate? just something to double check
There was a problem hiding this comment.
tau is the time, the value that is used in computations is Gamma, which is computed to be 1/tau
| @@ -95,7 +98,7 @@ def run(self, mp='cpu', chunk=False): | |||
| mp : {False, 'cpu', 'gpu'} (optional) | |||
| Select multiprocessing: False (or '' or None) means single-threaded. | |||
| 'gpu' indicates to use the CUDA implementation | |||
| Any other value which evaluates to `True` indicates cpu multiprocessed. | |||
| Any other value which evaluates to ``True`` indicates cpu multiprocessed. | |||
There was a problem hiding this comment.
this is nice
with the code as it is now, I'm fairly sure that windows machines will require an if __name__ == '__main__' to prevent infinite spawning---we might want to mention this in the docstring
I think that the "if name" windows oddity can be fixed by using the new futures library, as opposed to multiprocessing, we should probably open an issue to figure that out
There was a problem hiding this comment.
I am going to refrain from tackling this particular issue for the moment. We have an ongoing discussion in #18 regarding this windows forkbomb.
I'd like to test it out and decide on the desired course of action prior to doing this particular bit of documentation.
I have not attempted running on Windows, and would like to see the problem first.
|
I have added the 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. |
|
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 |
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