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

Enh workflow #84

Merged
merged 10 commits into from
Feb 27, 2023
Merged

Enh workflow #84

merged 10 commits into from
Feb 27, 2023

Conversation

asmacdo
Copy link
Collaborator

@asmacdo asmacdo commented Feb 7, 2023

#81

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@asmacdo asmacdo marked this pull request as ready for review February 17, 2023 20:36
Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

some suggestions, please rebase (should trigger codespell)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
```

Now we can "freeze" a container or containers that we plan to use so
anyone (including you) will know exactly which version was used. This is
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
anyone (including you) will know exactly which version was used. This is
anyone (including you) will know exactly which version was used and if you upgrade
the containers dataset to new version with newer versions of containers - you would need
to concisely choose to either upgrade this one or not while resolving conflicts. This is

README.md Outdated
Now we can "freeze" a container or containers that we plan to use so
anyone (including you) will know exactly which version was used. This is
technically optional, but very helpful. (All that changes is a
subproject commit hash.)
Copy link
Member

Choose a reason for hiding this comment

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

I have remembered one "cons" for this IMHO worth noting. Please incorporate something like

Suggested change
subproject commit hash.)
subproject commit hash.) A disadvantage though is that if you were to make
others to reproduce the results, you would need to share your clone of `///repronim/containers`
to share that updated state with frozen version in `.datalad/config`.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please re-read this section, I rewrote it with your ideas included.

README.md Outdated Show resolved Hide resolved
README.md Outdated

If everything worked as expected, we will now see our new analysis, and
a commit message of how it was obtained!

and now you have a dataset which has a git record on how these data
Copy link
Member

Choose a reason for hiding this comment

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

join into above or just make it capitalized

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

joined in and rewrote.

README.md Outdated Show resolved Hide resolved
Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Reads great, thank you! Just left few minor suggestions (with only one important one on needing to change .gitmodules if freezing)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
asmacdo and others added 5 commits February 24, 2023 12:35
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
@yarikoptic yarikoptic merged commit c666379 into ReproNim:master Feb 27, 2023
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

2 participants