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

815 more example data #2742

Merged
merged 10 commits into from
Mar 25, 2024
Merged

815 more example data #2742

merged 10 commits into from
Mar 25, 2024

Conversation

smk78
Copy link
Contributor

@smk78 smk78 commented Jan 18, 2024

Description

This PR adds the example data supplied in SasView/tutorials#13 into a folder example_data/magnetic_data.

This is, of course, where example_data used to live... A separate PR (SasView/sasdata#58) will add them to the sasdata repo.

Copy link
Contributor

@gonzalezma gonzalezma left a comment

Choose a reason for hiding this comment

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

I think this can be merged. Minor comments/questions concerning the help test for Steve to check.

src/sas/example_data/media/testdata_help.rst Outdated Show resolved Hide resolved
src/sas/example_data/media/testdata_help.rst Outdated Show resolved Hide resolved
src/sas/example_data/media/testdata_help.rst Outdated Show resolved Hide resolved
src/sas/example_data/media/testdata_help.rst Outdated Show resolved Hide resolved
src/sas/example_data/media/testdata_help.rst Outdated Show resolved Hide resolved
smk78 pushed a commit to SasView/sasdata that referenced this pull request Jan 25, 2024
@butlerpd
Copy link
Member

Two questions:

  1. Should all these example data be added here? or in sasdata?
  2. Should this also be merged into 6.0.0_release? for the upcoming beta release?

@smk78
Copy link
Contributor Author

smk78 commented Feb 12, 2024

Two questions:

  1. Should all these example data be added here? or in sasdata?
  2. Should this also be merged into 6.0.0_release? for the upcoming beta release?

To take the last question first, yes, we will want these changes in 6.0.0. This PR is against main on the assumption it will be merged into the release_6.0.0 branch before release?

SasView/sasdata#58 pushes these same changes (except for the coordinate & saved state examples) to sasdata. So in theory whether we build off main/release_6.0.0 or sasdata, the example data and testdata_help should be the same...

@butlerpd butlerpd added Discuss At The Call Issues to be discussed at the fortnightly call and removed Discuss At The Call Issues to be discussed at the fortnightly call labels Feb 13, 2024
@krzywon

This comment was marked as duplicate.

Copy link
Contributor

@krzywon krzywon left a comment

Choose a reason for hiding this comment

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

This has a lot of overlap with SasView/sasdata#58, but most of the overlap was meant for sasdata, not sasview. I think only the save states from here should stay and then it will be ready.

@butlerpd
Copy link
Member

Indeed @krzywon is right - most of these changed files no longer exist so we should not be overwriting the work of removing them unless there is a good reason. It looks in fact like all the changes to removed files were changed appropriately in SasView/sasdata#58 in which case we just need to revert everything here except changing the name of the save_state folder to save_states?

or did I miss something? will the scraping and bundling of docs occur properly?

@butlerpd butlerpd added the SasView 6.0.0 Required for 6.0.0 release label Mar 21, 2024
@smk78
Copy link
Contributor Author

smk78 commented Mar 22, 2024

Ok, so I note SasView/sasdata#58 got merged and I've looked at the example data in the release_0_9_0 branch and it looks good.

It is, however, missing the save_states folder and, more importantly, the coordinate_data because (SasView/sasdata#58 (comment)) those "are only importable through SasView". Whilst it wouldn't be the end of the world if we didn't include the save_states in the release, we do need to keep the coordinate_data.

There is also the question of testdata_help.rst; is that now a Sasdata file or still a SasView one (I couldn't see a /media folder in Sasdata so I'm assuming it's still SasView)? Note that the testdata_help.rst in this PR has updates.

So... @butlerpd @krzywon what's the best way to proceed here?

It looks like \magnetic_data, \other_formats & \reflectometry_data need deleting from SasView?

smk78 and others added 9 commits March 22, 2024 18:08
…o src/sas/example_data/magnetic_data/S50_15kG_horizSector_RF+.txt
…o src/sas/example_data/magnetic_data/S50_15kG_horizSector_RF-.txt
… src/sas/example_data/magnetic_data/S50_15kG_vertSector_RF+.txt
… src/sas/example_data/magnetic_data/S50_15kG_vertSector_RF-.txt
@butlerpd butlerpd changed the base branch from main to release_6.0.0 March 22, 2024 21:30
@butlerpd
Copy link
Member

This is now rebased against 6.0 release. Coordinate data and save project examples are the two folders remaining in sasview's example_data folder. The plan I believe is to move them over as well in a later release. In the meantime the \example_data from both repos are being merged when building the installer according to @krzywon so there should be no issue with losing the coordinate data.

Still needing addressing is the magnetic folder which looks to have been moved including these files so I guess that folder can just be removed from this branch and it will be ready to merge?

Copy link
Member

@butlerpd butlerpd left a comment

Choose a reason for hiding this comment

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

This is now ready to be merged according to all the reviews

@butlerpd butlerpd dismissed krzywon’s stale review March 25, 2024 20:07

This address all @krzywon concerns. Since he is out this week and we want to mover forward am resolving this for him

@butlerpd butlerpd merged commit b38fbf4 into release_6.0.0 Mar 25, 2024
36 checks passed
@butlerpd butlerpd deleted the 815-more-example-data branch March 25, 2024 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SasView 6.0.0 Required for 6.0.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants