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
[REL] add tags to changelog #4012
Conversation
👋 @Remi-Gau Thanks for creating a PR! Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft. Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.
For new features:
For bug fixes:
We will review it as quick as possible, feel free to ping us with questions if needed. |
Codecov Report
@@ Coverage Diff @@
## main #4012 +/- ##
=======================================
Coverage 91.77% 91.77%
=======================================
Files 134 134
Lines 15795 15795
Branches 3299 3299
=======================================
Hits 14496 14496
Misses 755 755
Partials 544 544
Flags with carried forward coverage won't be shown. Click here to find out more. 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -40,7 +40,7 @@ Enhancements | |||
|
|||
- :bdg-info:`Plotting` Surface plotting methods no longer automatically rescale background maps, which, among other things, allows to use curvature sign as a background map (:gh:`3173` by `Alexis Thual`_). | |||
|
|||
- :func:`~glm.first_level.first_level_from_bids` now takes an optional ``sub_labels`` argument and warns users of given subject labels that are not present in the dataset (:gh:`3351` by `Kevin Sitek`_). | |||
- :bdg-success:`API` :func:`~glm.first_level.first_level_from_bids` now takes an optional ``sub_labels`` argument and warns users of given subject labels that are not present in the dataset (:gh:`3351` by `Kevin Sitek`_). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we forgot one
doc/changes/latest.rst
Outdated
HIGHLIGHTS | ||
---------- | ||
|
||
- :bdg-info:`Plotting` Volume plotting functions like :func:`~plotting.plot_img` now have an optional ``radiological`` parameter, defaulting to ``False``. If ``True``, this will invert the x-axis and ``L`` and ``R`` annotations to confirm to radiological conventional view. (:gh:`3172` by `Konrad Wagstyl`_ and `Yasmin Mzayek`_). | ||
|
||
- :bdg-dark:`Code` Update Decoder objects to use the more efficient ``LogisticRegressionCV`` (:gh:`3736` by `Michelle Wang`_). | ||
|
||
- :bdg-success:`API` Add ``LassoCV`` as a new estimator option for Decoder objects (:gh: `3781` by `Michelle Wang`_) | ||
|
||
- :bdg-success:`API` Add ``vmin`` and ``symmetric_cbar`` arguments to :func:`~nilearn.plotting.plot_img_on_surf` (:gh:`3873` by `Michelle Wang`_). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can highlights be just repeats of some entries of the changelog or do they have to more generic rephrasings? I think I have seen both precedents.
Also this is my very personal take of what the highlights are. Happy to go with some others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just use best judgement. Sometimes it makes sense to rephrase or combine related PRs into one highlight. But otherwise just copy and paste is fine IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree on the highlights
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think that the incoming GLM and Surface Experimental features should be mentioned, no?
Once merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we should focus on #3856 ?
CONTRIBUTING.rst
Outdated
List of badges: | ||
|
||
.. codeblock:: rst | ||
|
||
:bdg-primary:`Doc` | ||
:bdg-secondary:`Maint` | ||
:bdg-success:`API` | ||
:bdg-info:`Plotting` | ||
:bdg-warning:`Test` | ||
:bdg-danger:`Deprecation` | ||
:bdg-dark:`Code` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if contributors forget them at least we will have them listed somewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally wanted to put this in https://nilearn.github.io/dev/maintenance.html. The idea was that this would be the responsibility of the maintainer during a release so as to not add an extra "burden" to contributors. But maybe it's ok since people seem to already include it sometimes. WDYT? If we keep it here I still don't see a problem with merging a PR without it and then just checking before a release. This should be easy enough since we have to manipulate the file anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah that's true. I remember now. Sorry about that.
I would say that contributors are likely to do what is alreay in the changelog rather than read the doc, so we always have it from the start or only at the end.
I would prefer to say always from the start also because doing it much later I ran into the problem of asking mysef "was that a code change or an API change?" because some of the context of the change was lost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok let's keep it here then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely something we can revisit and change if we see that this approach does not work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM so far.
Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>
Changes proposed in this pull request: