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 #78

Merged
merged 25 commits into from
Oct 30, 2018
Merged

Miscellaneous changes #78

merged 25 commits into from
Oct 30, 2018

Conversation

T-Nicholls
Copy link
Contributor

A number of miscellaneous changes that I have compiled whilst using AT, plus the commits from @willrogers' pull request #47.

@@ -76,7 +76,7 @@ def find_orbit4(ring, dp=0.0, refpts=None, guess=None, **kwargs):
delta_matrix = numpy.zeros((6, 5), order='F')
for i in range(4):
delta_matrix[i, i] = step_size
id4 = numpy.asfortranarray(numpy.identity(4))
id4 = numpy.asfortranarray(numpy.eye(4))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why you've changed identity to eye here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it was causing a problem with our threading in atip and identity just imports and calls eye anyway. So I figured there was no downside to changing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Except it was renamed to identity because it is a clearer name. I vote to reinstate it because it is easier to understand.

Copy link
Contributor

@lfarv lfarv Oct 25, 2018

Choose a reason for hiding this comment

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

I have no problem with eye since it's the name for the similar Matlab function, but I let you, python experts, decide.

@willrogers willrogers requested a review from lfarv October 25, 2018 16:20
Copy link
Contributor

@lfarv lfarv left a comment

Choose a reason for hiding this comment

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

in atmexall.m

% Add AT directories to the Matlab path
 atpath

This results in a double definition of all the Matlab path ! It is harmful if AT is correctly installed, and it is is not, I do not see how you call atmexall. More generally, I do not like modifications of the Matlab path without the user's intervention: it can result in unexpected files being called instead of the desired ones…

@willrogers
Copy link
Contributor

willrogers commented Oct 26, 2018

I think you mentioned that Matlab path problem before. The misunderstanding comes because running atpath is the way I use AT - perhaps you set the path by default at Matlab startup or something. But I don't mind, we will revert that commit.

Your point about modifying the path is sensible.

@lfarv
Copy link
Contributor

lfarv commented Oct 26, 2018

Yes Will, you can run atpath to setup AT, but then you are responsible to call it. In this case, you know what you are doing, and then the path is lost at the end of the Matlab session, which is a protection against the risk I mentioned. But I note that the 'install' document still does not mention the different ways to correctly setup the Matlab path, either with the GUI, or with the startup.m file, or temporarily as you are doing. I also think we already discussed that, but the outcome has been lost…
Here is what I propose for point 4 in INSTALL.md

  1. Insert recursively the directories at_installation/atmat and at_installation/atintegrators in the Matlab path. This can be done by
  • using the GUI: open the "Set Path" window, press "Add with subfolders", select at_installation/atmat. Repeat the operation for at_installation/atintegrators,
  • using the startup.m file: insert a line addpath(genpath(at_installation/atmat) and a similar one for atintegrators
  • temporarily modifying the path by running:
>> cd  at_installation/atmat
>> atpath

@T-Nicholls
Copy link
Contributor Author

New changes:

  • Reverted eye to identity.
  • Removed atpath from atmexall.m.
  • Replaced point 3(Start Matlab.) with @lfarv's proposed new point in INSTALL.md and made small formatting adjustments.
  • Updated the pyat readme to avoid strange behaviour relating to .pyc and .so files, and no longer mix develop and install, as discussed with @willrogers.

Are there any other changes that people would like added?

@willrogers willrogers mentioned this pull request Oct 29, 2018
Closed
@T-Nicholls
Copy link
Contributor Author

The Corrector bug caused all correctors to have a length of 0, this caused some discrepancies in twiss data if a lattice had elements of class Correcor with a non-zero length.

@T-Nicholls T-Nicholls mentioned this pull request Oct 29, 2018
@lfarv
Copy link
Contributor

lfarv commented Oct 29, 2018 via email

@T-Nicholls
Copy link
Contributor Author

@lfarv @willrogers
INSTALL.md should now be correct, and following it works as intended for me.
I think it's now ready to merge.

@willrogers willrogers merged commit db79b2e into atcollab:master Oct 30, 2018
@T-Nicholls T-Nicholls deleted the miscChanges branch November 7, 2018 17:36
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