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

Beta release 2024 #573

Merged
merged 23 commits into from
Oct 15, 2024
Merged

Beta release 2024 #573

merged 23 commits into from
Oct 15, 2024

Conversation

MBartkowiakSTFC
Copy link
Collaborator

@MBartkowiakSTFC MBartkowiakSTFC commented Oct 10, 2024

Description of work
Final changes before the beta release of MDANSE 2.

closes #580
closes #579
closes #577
closes #576
closes #582
closes #492

Fixes

  1. Updated version numbers of MDANSE and MDANSE_GUI,
  2. Splash screen text changed to 'beta release',
  3. Correction to the job progress bar, to ensure that a finished job is 100% complete.
  4. Initialised the primary axis field in the plotting GUI with the correct axis name.
  5. Correction to the Grid.py file, allowing to plot curves with different primary axes.

To test
Run a DOS job on a trajectory. Check that it reached 100% when finished.
Plot the results of DOS and VACF at the same timer. Switch the plot to Grid. All selected curves should be visible.

@ChiCheng45
Copy link
Collaborator

ChiCheng45 commented Oct 14, 2024

Looks, good so far.

I've found one issue with the project coordinates configurator. For the axial and planar projection, you can input the vector of zeros, this causes the jobs to fail. Also, non-numerical numbers can be entered into the boxes, hitting run doesn't do anything but we should stop the user from entering these like the other configurators.

image

@MBartkowiakSTFC
Copy link
Collaborator Author

The projection widget should work correctly now.

@ChiCheng45
Copy link
Collaborator

So far I have found some minor issues (#577, #576, #575 and #574) which I think we need not fix for this release.

There is a lammps trajectory conversion failure with the p_lammps_10000 test file from one drive. This does not work on protos either and the issue appears to have been introduced in #546. I tested MDANSE at a98af80 and the conversion works fine at this point. I think we should try to fix this before the release.

@MBartkowiakSTFC
Copy link
Collaborator Author

The problem is that LAMMPS has multiple possible formats of the configuration file, and specifically this section:
https://docs.lammps.org/atom_style.html
is what creates the problem. Depending on which one is selected in the LAMMPS script, the number of columns in the Atoms section and their meaning will be different.

Some of the files conveniently contain a comment after the word 'Atoms' which specified which atom_style is being used. In the long run we should probably aim to support all of them, but there are quite a few possibilities. We can't really rely on the comment being there, and being correct, so some other mechanism of recognising the atom_style will have to be implemented.

Having said that, I think I fixed the problem with our example trajectory. It should work again.

@ChiCheng45
Copy link
Collaborator

It looks good all my lammps test trajectories work fine now.

Can we update the self-part of the van Hove function so that it gives the same error as the distinct part when a simulation without a unit cell is used?

This is error in the self-part

179, in run_step
.unit_cell.volume
^^^^^^^^^
AttributeError: 'RealConfiguration' object has no attribute 'unit_cell'

This is the error with the distinct-part

line 81, in detailed_unit_cell_error
raise ValueError(
ValueError: This analysis job requires a unit cell (simulation box) to be defined. The box will be used for calculating density in the analysis. You can add a simulation box to the trajectory using the TrajectoryEditor job. Be careful adding the simulation box, as the wrong dimensions can render the results meaningless.

@ChiCheng45
Copy link
Collaborator

I have identified two new issues we may want to try to fix #579 and maybe #581. For #581 It might be better to deactivate the unfolded trajectory job for this release.

@MBartkowiakSTFC
Copy link
Collaborator Author

Thanks for finding these problems! In the meantime I added the error message to the van Hove self function.

Also, I think that the commits I added today should close #580 #576 #577 and make the AverageStructure analysis fail sooner, which does not solve the problem, but is an improvement.

@ChiCheng45
Copy link
Collaborator

ChiCheng45 commented Oct 15, 2024

Looks like #580, #576 and #577 are fixed now.

Still one minor visual issue with the q_vector generator is that when you use a vector or range of 4 or more elements the cell in the table is not highlighted. The run button is blocked so it is not much of an issue, so I think we can leave this as it is.

image

@MBartkowiakSTFC
Copy link
Collaborator Author

To address #582 I added placeholder text to the instrument input fields. The values are not very useful (they are all 1) but at least this produces a valid configuration and it is clear what values were used.

@MBartkowiakSTFC
Copy link
Collaborator Author

Also, #579 should be fixed now.
#581 has been addressed by disabling UnfoldedTrajectory for now.

@ChiCheng45 ChiCheng45 self-requested a review October 15, 2024 14:41
Copy link
Collaborator

@ChiCheng45 ChiCheng45 left a comment

Choose a reason for hiding this comment

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

Looks great, I think this is ready to be released.

@MBartkowiakSTFC MBartkowiakSTFC merged commit 9b82b6d into protos Oct 15, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment