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.

Sign up for GitHub

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

Jump to bottom

Analysis functions for diatomic molecules. #329

Merged
merged 32 commits into from
Apr 13, 2024
Merged

Analysis functions for diatomic molecules. #329

merged 32 commits into from
Apr 13, 2024

Conversation

Alexsp32
Copy link
Member

@Alexsp32 Alexsp32 commented Jan 5, 2024

Changes:

  • Added new DynamicsOutputs for Desorption Angles and DynamicsVariables during desorption (New DynamicsOutputs: OutputDesorptionTrajectory, OutputDesorptionAngle #317)
  • Added distance, centre of mass, reduced mass function with and without check for closer periodic copies to a Structure submodule.
  • Added an Analysis submodule to contain functions needed for more complex analysis of MD trajectories.

@Alexsp32
Add Analysis submodule and diatomic molecule analysis functions.
85c0ae0 
- Added new DynamicsOutput: OutputDesorptionTrajectory #317
- Added new DynamicsOutput: OutputDesorptionAngle #317
- Created an `Analysis` submodule for common analysis functions
@Alexsp32
Add Structure submodule from NQCBase
c354fc7
@Alexsp32
Forgot to export some DynamicsOutputs
66761d8
@Alexsp32
Fixed a few self-references
522a879
@Alexsp32
Analysis should load properly now
3f13330
@Alexsp32
Cleanup
1f42249 
Tried to fix assignment of quantise_diatomic in Analysis

Second attempt

Removed superfluous import of PyCall

Another missing import

Add missing Simulation variants of functions

Maybe it's dot after all?

Stupid Any overload

Second attempt
@Alexsp32 Alexsp32 linked an issue Jan 5, 2024 that may be closed by this pull request
New DynamicsOutputs: OutputDesorptionTrajectory, OutputDesorptionAngle #317
Closed
@Alexsp32 Alexsp32 marked this pull request as ready for review January 12, 2024 11:43
@Alexsp32 Alexsp32 changed the title Draft: Basic analysis functions for diatomic molecules. Analysis functions for diatomic molecules. Jan 15, 2024
Alexsp32 added a commit that referenced this pull request Jan 15, 2024
@Alexsp32
Version bump for #325 and #329
54ab1e6
reinimaurer1
reinimaurer1 reviewed Jan 15, 2024
View reviewed changes
Copy link
Member

@reinimaurer1 reinimaurer1 left a comment

Choose a reason for hiding this comment

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

This looks good, although a lot of the utility functions for the output aren't covered by tests. Might be worth to cover the ones that don't to completely drivial things

@Alexsp32 Alexsp32 marked this pull request as draft February 9, 2024 10:17
@Alexsp32
Copy link
Member Author

Alexsp32 commented Feb 9, 2024

Need to re-work several bits of this PR since they're very memory-inefficient and some edge cases aren't covered.

@Alexsp32 Alexsp32 self-assigned this Apr 8, 2024
@Alexsp32
Copy link
Member Author

Alexsp32 commented Apr 8, 2024 

DesorptionFrame - No views: 1.191380 seconds (7.33 M allocations: 486.175 MiB, 6.54% gc time, 84.50% compilation time: <1% of which was recompilation)
DesorptionFrame - With views: 0.401479 seconds (4.67 M allocations: 309.999 MiB, 6.78% gc time, 58.73% compilation time)
DesorptionAngle - No views: 0.645341 seconds (5.03 M allocations: 335.521 MiB, 4.93% gc time, 73.09% compilation time: 12% of which was recompilation)
DesorptionAngle - With views: 0.446124 seconds (4.71 M allocations: 313.256 MiB, 4.90% gc time, 61.16% compilation time)

I've merged in various changes to decrease the memory footprint of OutputDesorptionAngle and OutputDesorptionTrajectory and added a unit test for an example system, so I think this is now ready to be merged.

@Alexsp32 Alexsp32 marked this pull request as ready for review April 8, 2024 10:31
@reinimaurer1
Copy link
Member

Nicely done!

@Alexsp32
Copy link
Member Author

Alexsp32 commented Apr 8, 2024

Docs preview here

@reinimaurer1 reinimaurer1 self-requested a review April 12, 2024 13:13
reinimaurer1
reinimaurer1 reviewed Apr 12, 2024
View reviewed changes
src/structure.jl
@@ -0,0 +1,183 @@
module Structure
Copy link
Member

Choose a reason for hiding this comment

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

can you please create a markdown file for structure.md in docs/API to explain in a few sentences what this module is about

Copy link
Member Author

Choose a reason for hiding this comment

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

I've slightly improved docs here.

@reinimaurer1
Copy link
Member

@wgst there are a few new functions that could be useful for you

@Alexsp32 Alexsp32 merged commit 12a9980 into main Apr 13, 2024
11 of 12 checks passed
@Alexsp32 Alexsp32 deleted the diatomicbasics branch April 13, 2024 11:24
Alexsp32 added a commit that referenced this pull request Apr 15, 2024
@Alexsp32
Analysis functions for diatomic molecules. (#329)
8d58c57 
- Added new DynamicsOutput: OutputDesorptionTrajectory #317
- Added new DynamicsOutput: OutputDesorptionAngle #317
- Created an `Analysis` submodule for common analysis functions

-  Add `Structure` submodule for common utility functions for atomic structure. 

* Add rudimentary tests for NQCDynamics.Structure

* Improved method definitions, possible efficiency gain by using views

* Add unit tests for diatomic analysis functions for desorption

* Add "Analysis" group to unit testing

* Added explanation for Structure module scope
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reinimaurer1 reinimaurer1 reinimaurer1 approved these changes

@Nhertl Nhertl Awaiting requested review from Nhertl

@jamesgardner1421 jamesgardner1421 Awaiting requested review from jamesgardner1421

Assignees

@Alexsp32 Alexsp32

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New DynamicsOutputs: OutputDesorptionTrajectory, OutputDesorptionAngle
2 participants
@Alexsp32 @reinimaurer1