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

Miscellaneous changes to pyat #103

Merged
merged 23 commits into from
Apr 2, 2019
Merged

Conversation

T-Nicholls
Copy link
Contributor

A number of changes from my todo list for AT.

A rough summary of the changes:

  • All remaining uses of multi_dot have been converted to .dot so that we are consistent throughout pyat.
  • I have updated ohmi_envelope to work with any pyat lattice description.
  • Improved the fixtures for both normal tests and Matlab comparison tests.
  • Fixed typos, and made some readability and PEP8 conformity changes throughout.
  • Added a check to sanitise_class to ensure the specified pass method exists, also added warnings in find_class_name if no PassMethod is given or if the one given doesn't end in 'Pass'.
  • Added new tests for most of the untested sections in pyat, most notably lattice object and elements; now almost all of pyat is covered.
  • Ensure the radiation state of get_mcf when it is called on the lattice, as we do for linopt and ohmi_envelope.
  • Updated our numpy minimum version to 1.13.0 as our use of record arrays is broken in 1.12.1; I didn't bother checking if it worked in versions >1.10 <1.12.1 though.

Some questions that I still have:

  • In get_twiss and linopt if get_chrom is False then we:
    chrom = None
    dispersion = numpy.NaN
    disp0 = numpy.NaN
    
    why the difference between chrom and dispersion surely if we are not calculating either then they should both be None or numpy.NaN?
  • We have at.lattice.utils and at.load.utils, they are not in the same directory so there is not an immediate issue, however, because of pyat's import structure at.utils is valid and returns the lattice utils; is this ambiguity advised or could one of them be sensibly renamed?
  • As there are quite a few changes to pyat now on pull requests I will wait until they have all been merged before I go through and make significant improvements to pyat's PEP8 conformity, so as to avoid having to deal with conflicts.

@T-Nicholls
Copy link
Contributor Author

T-Nicholls commented Mar 29, 2019

On the note of the numpy bug, it is caused by us trying to create an empty record array in ohmi_envelope using numpy.rec.fromrecords when no refpts are passed. The IndexError that it caused in numpy was a known bug and was fixed in #8486; if we don't want to bump the numpy requirement then we could add a workaround, something like:

    if uint32refs.shape == (0,):
        data = numpy.recarray((0,), dtype=ENVELOPE_DTYPE)
    else:
        data = numpy.rec.fromrecords(
            list(map(propag, ms, bbcum[uint32refs], orbs[uint32refs, :])),
            dtype=ENVELOPE_DTYPE)

Edit: I forgot to mention one of our internal modules is pinned to numpy 1.11.1, hence the desire for a workaround.

@willrogers
Copy link
Contributor

We actually do have something that depends on the older version of numpy, so I think we should use this workaround for the time being (with a comment that explains it).

pyat/at/lattice/lattice_object.py Outdated Show resolved Hide resolved
'../../integrators',
pass_method + extension))
if not os.path.exists(file_path):
raise AttributeError('PassMethod {0} does not exist.'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should report the element index (if available) in the error message, like it is done in err_message

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated err_message so that it can be used here as well.

pyat/at/physics/radiation.py Outdated Show resolved Hide resolved
@lfarv
Copy link
Contributor

lfarv commented Mar 30, 2019

The generalization of ohmi_envelope to any sequence of elements is a very good point !

Concerning chromaticity and dispersion values: pushing None into the record array results in array([nan, nan, nan, nan]) ! So I suggest setting array([nan, nan]) for the chromaticity

warn(AtWarning('Inconsistent energy values, '
'"energy" set to {0}'.format(energy)))
attributes['energy'] = energy
attributes['energy'] = get_ring_energy(elems)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a problem here:
I made significant modifications to lattice_object in the plot_preparation branch. To avoid unnecessary list copies (build a list first, and then give it to super(Lattice, self).__init__, the list constructor, which will itself build a copy of this list), full_scan now accepts any iterable as input and returns an iterator over the elements. So it is executed within super(Lattice, self).__init__. The gain is most effective when the input to full_scan is itself an iterator. In such a case, after the first loop over elements, get_ring_energy will get an empty iterator, and fail.

Example: we have a Lattice ring, without RingParam element but with an RFCavity giving the energy. Let's assume we want to filter this lattice with some condition:

ring2=Lattice([elem for elem in ring if <some condition>])
Here we build a list first, which will be copied by the list constructor. get_ring_energy will work.

ring2=Lattice(elem for elem in ring if <some condition>)
Here we have no intermediate list, <some condition> will be tested within the list constructor and get_ring_energy will fail.

I think we should keep the possibility of accepting any iterable (sequence or iterator) as input, not only a sequence.

For the time being, I suggest to keep lattice_object unchanged to avoid merging problems. Keep all your other modifications as they are, let's merge both branches and start over then with this code factorisation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will revert the modifications to lattice_object for now. Yes, I am learning that working on multiple branches at once is a recipe for problems, the test on #102 is failing because it relies on changes made here.

lfarv added a commit that referenced this pull request Apr 2, 2019
@T-Nicholls
Copy link
Contributor Author

I am now happy for this to merge, does anyone have anything else that they would like to add?

@lfarv
Copy link
Contributor

lfarv commented Apr 2, 2019

nothing else to say, go on with merging!

if (pass_method == 'IdentityPass') and (length != 0.0):
raise AttributeError(err_message("length {0}.", length))
extension = sysconfig.get_config_vars().get('EXT_SUFFIX', '.so')
file_path = os.path.realpath(os.path.join(__file__,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a bit brittle to me. Could you import the module instead, or is there a reason you don't want to do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever method we use we have to do all this messy filepath finding.
If we do a try except to open and read the file then we have to catch different errors, IOError in python 2 and FileNotFoundError in python 3. I just thought that was messier than os.path.exists.

from at.lattice import Lattice, AtWarning, AtError


def test_lattice_creation():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these tests are really two or three tests bundled into one.

It's probably better to split them. You end up with very long test names, but people do like that style:
http://blog.socosomi.com/dont-be-afraid-of-long-test-names/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll split some of them down.

@willrogers
Copy link
Contributor

I made a couple of comments, but generally looks good, thanks.

@willrogers willrogers merged commit 7d782d4 into atcollab:master Apr 2, 2019
@T-Nicholls T-Nicholls deleted the miscChanges branch April 2, 2019 15:44
lfarv added a commit that referenced this pull request Apr 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants