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

IBS Table Fix and Code Source Comments #1210

Merged
merged 9 commits into from
Jan 15, 2024

Conversation

fsoubelet
Copy link
Contributor

Hello,

Since I have looked at the IBS module source code (ibsdb.f90) quite a lot for work recently, I figured it would be beneficial to submit a PR with an "improved" (as much as I could) documentation of this module.

Important point: No code change is made in this PR.

All this PR brings is a lot of comments trying to explain:

  • What do things correspond to (variables, mostly)
  • What is being done
  • Why it is being done (sometimes)

I'm not 100% sure I am correct on everything in here, but almost all my additions should be correct and, hopefully, helpful.

Copy link
Contributor

@rdemaria rdemaria left a comment

Choose a reason for hiding this comment

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

ok to me, waiting for the tests to run

@fsoubelet
Copy link
Contributor Author

I've made just a few corrections / additions.

Interesting to note, that I forgot to mention initially: it seems (empirically, to me) that the IBS table is only created internally when the command is called with a value for an output file (ibs, file="string";) and not otherwise. This is note what is written in the user guide (currently, end of page 198) which implies the table is always created.

@rdemaria
Copy link
Contributor

Feel free to create the table anyway if it is helpful for your use case.

@fsoubelet
Copy link
Contributor Author

fsoubelet commented Dec 19, 2023

Hi @rdemaria, here are a few more changes then:

  • I have forced the IBS table to be written, always. To do so,
    • I have removed the set_option call in mad_ibs.c (line 26 of this file in my PR)
    • I have removed the check just below, that guarded the creation of the table (used to only create if export to file was demanded by the user).
    • I have removed the checking for the ibs_table flag in the fortran subroutine (used to be line 128), as well as the if clause that guarded writing to the table (used to be lines 216 and 232).

This way when calling ibs; in MAD-X, the table is ALWAYS created by the C module and ALWAYS written to by the fortran subroutine. The export to file is still guarded by a check and only triggered if the command is invocked with file="string";. This corresponds now to the behaviour that is described in the user guide.

  • I have made a small addition (lines 554-556 in my PR) so that both the coulomb logarithm and the full constant of Eq (8) of the note are exported to the global namespace, as ibs.coulog and ibs.const, respectively.

A quick local build shows that all of this works, but let's run the tests to be sure. I could also make a small addition to the IBS section in the user guide repo to specify one might access the coulomb log and the common constant the same as the growth rates. Should I?

@fsoubelet fsoubelet requested a review from rdemaria December 20, 2023 12:28
@rdemaria
Copy link
Contributor

Yes please add two lines to the manual if it.is easy to do so...

@fsoubelet fsoubelet changed the title IBS Source Comments IBS Fix and Code Source Comments Dec 21, 2023
@fsoubelet fsoubelet changed the title IBS Fix and Code Source Comments IBS Table Fix and Code Source Comments Dec 21, 2023
@fsoubelet
Copy link
Contributor Author

fsoubelet commented Dec 21, 2023

Done! I have also brought in a little correction about the txi, tyi and tli terms found in the IBS table, since it was mentioned that they are the "intermediate" growth rates but they are not.

Let me know if this looks alright to you @rdemaria

@fsoubelet
Copy link
Contributor Author

@rdemaria I'm not sure what went wrong with this single Windows build, but it seems it received an interrupt signal due to the runtime timeout of 30min defined in the workflow file (currently line 176).

Would it be possible to try reruning just this one? I'm fairly confident it would pass just fine.

@fsoubelet
Copy link
Contributor Author

@rdemaria I had to fix a mistake I had made in the documentation, sorry. The tests passed fine when you reran, could you approve once more?

@rdemaria rdemaria merged commit 6a48dad into MethodicalAcceleratorDesign:master Jan 15, 2024
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.

2 participants