Fix clims when plotting shapes element annotations with matplotlib rendering - #368

Merged
melonora merged 11 commits into
scverse:mainfrom
melonora:issue_#328
Oct 10, 2024
Merged

Fix clims when plotting shapes element annotations with matplotlib rendering#368
melonora merged 11 commits into
scverse:mainfrom
melonora:issue_#328

Conversation

@melonora

@melonoramelonora commented Oct 3, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

closes#324

This PR fixes the issue described in #324. When creating the patch collection a default Normalize was always created instead of using the one provided by the user. Furthermore in this PR some legacy code was removed which did not do anything.

@melonoramelonora changed the title Fix clims when plotting shapes element annotationsFix clims when plotting shapes element annotations with matplotlib renderingOct 3, 2024
@codecov-commenter

codecov-commenter commented Oct 4, 2024•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 54.54545% with 5 lines in your changes missing coverage. Please review.

Project coverage is 83.73%. Comparing base (6ffe22b) to head (16a5a38).
Report is 1 commits behind head on main.

Files with missing linesPatch %Lines
src/spatialdata_plot/pl/basic.py50.00%4 Missing ⚠️
src/spatialdata_plot/pl/utils.py66.66%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #368 +/- ##
==========================================
- Coverage 83.76% 83.73% -0.04% 
==========================================
Files 8 8 Lines 1540 1543 +3 ==========================================
+ Hits 1290 1292 +2 - Misses 250 251 +1
Files with missing linesCoverage Δ
src/spatialdata_plot/pl/utils.py76.37% <66.66%> (+0.20%)⬆️
src/spatialdata_plot/pl/basic.py89.26% <50.00%> (-1.60%)⬇️

@melonora
melonora marked this pull request as ready for review October 4, 2024 08:36
@melonora

melonora commented Oct 4, 2024

Copy link
Copy Markdown
ContributorAuthor

There were some tests that had an expected figure that was wrong in the first place so I fixed that. Think it is good to go for now. Here and there in the test code base I also saw no copies of anndata being created, spamming warnings so I silenced those.

@melonoramelonora mentioned this pull request Oct 4, 2024
Closed
LucaMarconato
LucaMarconato requested changes Oct 4, 2024
View reviewed changes
Comment threadsrc/spatialdata_plot/pl/utils.py
elif vcenter is None:
norm = Normalize(vmin=vmin, vmax=vmax, clip=True)
else:
norm = TwoSlopeNorm(vmin=vmin, vmax=vmax, vcenter=vcenter)

@LucaMarconatoLucaMarconatoOct 4, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now that vcenter is removed, it should be removed also from the function signature. Also, kwargs is in the signature but not used, so I would remove it.

Comment threadsrc/spatialdata_plot/pl/utils.py
else:
try:
norm = colors.Normalize(vmin=min(c), vmax=max(c))
norm = colors.Normalize(vmin=min(c), vmax=max(c)) if norm is None else norm

@LucaMarconatoLucaMarconatoOct 4, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In this function Normalize() is initialized without clip, while in _prepare_cmap_norm() the default is to set clip=True. I would choose one of the two as our default choice. The user will be able to specify clip, vcenter, etc by passing a norm object directly.

@melonoramelonoraOct 4, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

hmm let me double check that if we don't pass norm as user, whether ultimately the norm is always created anyway, then we can get rid of normalize instance initiated here.

@melonoramelonoraOct 4, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ok there is code left over of when vmin and vmax were removed. Not certain whether to address this in a different PR.

@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd address the choice of the value of clip in this PR please, because it's easy to forget about this in a new PR.

@melonoramelonoraOct 9, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

default is set to False now

Comment threadtests/_images/Labels_can_render_given_scale_of_multiscale_labels.png
Comment threadtests/pl/test_render_shapes.py
Comment threadtests/pl/test_render_shapes.py
LucaMarconato
LucaMarconato reviewed Oct 9, 2024
View reviewed changes
Comment threadsrc/spatialdata_plot/pl/utils.py
cmap: Colormap | str | None = None,
norm: Normalize | None = None,
na_color: ColorLike | None = None,
vmin: float | None = None,

@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@timtreis I don't remember the outcome of the discussion with the user that reported this. Is this the way to go (=letting users only use norm and not vmin, vmax) or shall we remove vcenter only and keep vmin, vmax and use them to initialize the default Normalize object?

@melonoramelonoraOct 9, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

In the discussion it was stated that vmin and vmax are removed. This function is only internally called

@melonoramelonoraOct 9, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

So we agree on clip 'True' by default if user does not provide normaloze object?

@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If vmin and vmax are not exposed to the user (and hence they are None), then clip will have no effect because when exposed vmin, vmax are None, the data limits are used. So I would keep the default clip to be False (which is matplotlib's default).

@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One thing, if vmin, vmax are removed from pl.render_shapes(), we should throw an informative exception or deprecation warning, explaining to the user that norm should be used instead. Could you add that please?

@melonoramelonoraOct 9, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

They have not been removed in this PR though and the public functions thus already did not contain these parameters

@melonoramelonoraOct 9, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I did add a deprecation warning in case of the arguments being passed as kwargs

@LucaMarconato

LucaMarconato commented Oct 9, 2024•
edited by melonora
Loading

Copy link
Copy Markdown
Member

Thanks @melonora, pre-approving. I kindly ask you just two minor adjustments:

  • please add the deprecation warning for vmin, vmax also in the other 3 render functions.
  • please modify the warning saying to pass the Normalize object to norm, otherwise it may not be clear how to use it.
    After this feel free to merge thanks.

@melonora

melonora commented Oct 10, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks @melonora, pre-approving. I kindly ask you just two minor adjustments:

  • please add the deprecation warning for vmin, vmax also in the other 3 render functions.
  • please modify the warning saying to pass the Normalize object to norm, otherwise it may not be clear how to use it.
    After this feel free to merge thanks.

added todo as well to add this to tutorial notebook.

@melonora
melonora merged commit 6cce44e into scverse:mainOct 10, 2024
@melonora
melonora deleted the issue_#328 branch October 10, 2024 11:31
@timtreistimtreis mentioned this pull request May 8, 2026
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LucaMarconatoLucaMarconatoLucaMarconato approved these changes
@timtreistimtreisAwaiting requested review from timtreis

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unable to set vmin vmax when plotting vector data

3 participants

@melonora@codecov-commenter@LucaMarconato
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Fix clims when plotting shapes element annotations with matplotlib rendering - #368
Merged
melonora merged 11 commits into
scverse:mainfrom 
melonora:issue_#328
Oct 10, 2024
Merged
Fix clims when plotting shapes element annotations with matplotlib rendering#368
melonora merged 11 commits into
scverse:mainfrom 
melonora:issue_#328
Conversation
@melonora
@melonoramelonora
commented
Oct 3, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
closes#324
This PR fixes the issue described in #324. When creating the patch collection a default Normalize was always created instead of using the one provided by the user. Furthermore in this PR some legacy code was removed which did not do anything.
@melonoramelonora
changed the title
Fix clims when plotting shapes element annotationsFix clims when plotting shapes element annotations with matplotlib renderingOct 3, 2024
@codecov-commenter
codecov-commenter
commented
Oct 4, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 54.54545% with 5 lines in your changes missing coverage. Please review.
Project coverage is 83.73%. Comparing base (6ffe22b) to head (16a5a38).

Report is 1 commits behind head on main.
Files with missing linesPatch %Lines
src/spatialdata_plot/pl/basic.py50.00%4 Missing ⚠️
src/spatialdata_plot/pl/utils.py66.66%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #368 +/- ##
==========================================
- Coverage 83.76% 83.73% -0.04% 
==========================================
Files 8 8 Lines 1540 1543 +3 ==========================================
+ Hits 1290 1292 +2 - Misses 250 251 +1 
Files with missing linesCoverage Δ
src/spatialdata_plot/pl/utils.py76.37% <66.66%> (+0.20%)⬆️
src/spatialdata_plot/pl/basic.py89.26% <50.00%> (-1.60%)⬇️
@melonora
melonora
marked this pull request as ready for review
October 4, 2024 08:36
@melonora
melonora
commented
Oct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
There were some tests that had an expected figure that was wrong in the first place so I fixed that. Think it is good to go for now. Here and there in the test code base I also saw no copies of anndata being created, spamming warnings so I silenced those.
@melonoramelonora
mentioned this pull request
Oct 4, 2024

Closed

LucaMarconato
LucaMarconato
requested changes
Oct 4, 2024
View reviewed changes
Comment threadsrc/spatialdata_plot/pl/utils.py
 elif vcenter is None:
 norm = Normalize(vmin=vmin, vmax=vmax, clip=True)
 else:
 norm = TwoSlopeNorm(vmin=vmin, vmax=vmax, vcenter=vcenter)
@LucaMarconatoLucaMarconatoOct 4, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Now that vcenter is removed, it should be removed also from the function signature. Also, kwargs is in the signature but not used, so I would remove it.
Comment threadsrc/spatialdata_plot/pl/utils.py
 else:
 try:
 norm = colors.Normalize(vmin=min(c), vmax=max(c))
 norm = colors.Normalize(vmin=min(c), vmax=max(c)) if norm is None else norm
@LucaMarconatoLucaMarconatoOct 4, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

In this function Normalize() is initialized without clip, while in _prepare_cmap_norm() the default is to set clip=True. I would choose one of the two as our default choice. The user will be able to specify clip, vcenter, etc by passing a norm object directly.
@melonoramelonoraOct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

hmm let me double check that if we don't pass norm as user, whether ultimately the norm is always created anyway, then we can get rid of normalize instance initiated here.
@melonoramelonoraOct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ok there is code left over of when vmin and vmax were removed. Not certain whether to address this in a different PR.
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'd address the choice of the value of clip in this PR please, because it's easy to forget about this in a new PR.
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

default is set to False now
Comment threadtests/_images/Labels_can_render_given_scale_of_multiscale_labels.png
Comment threadtests/pl/test_render_shapes.py
Comment threadtests/pl/test_render_shapes.py
LucaMarconato
LucaMarconato
reviewed
Oct 9, 2024
View reviewed changes
Comment threadsrc/spatialdata_plot/pl/utils.py
 cmap: Colormap | str | None = None,
 norm: Normalize | None = None,
 na_color: ColorLike | None = None,
 vmin: float | None = None,
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@timtreis I don't remember the outcome of the discussion with the user that reported this. Is this the way to go (=letting users only use norm and not vmin, vmax) or shall we remove vcenter only and keep vmin, vmax and use them to initialize the default Normalize object?
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In the discussion it was stated that vmin and vmax are removed. This function is only internally called
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

So we agree on clip 'True' by default if user does not provide normaloze object?
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If vmin and vmax are not exposed to the user (and hence they are None), then clip will have no effect because when exposed vmin, vmax are None, the data limits are used. So I would keep the default clip to be False (which is matplotlib's default).
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

One thing, if vmin, vmax are removed from pl.render_shapes(), we should throw an informative exception or deprecation warning, explaining to the user that norm should be used instead. Could you add that please?
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

They have not been removed in this PR though and the public functions thus already did not contain these parameters
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I did add a deprecation warning in case of the arguments being passed as kwargs
@LucaMarconato
LucaMarconato
commented
Oct 9, 2024•

edited by melonora

Loading

Copy link


Copy Markdown

Member
Thanks @melonora, pre-approving. I kindly ask you just two minor adjustments:
  •  please add the deprecation warning for vmin, vmax also in the other 3 render functions.
  •  please modify the warning saying to pass the Normalize object to norm, otherwise it may not be clear how to use it.
    
After this feel free to merge thanks.
@melonora
melonora
commented
Oct 10, 2024

Copy link


Copy Markdown

ContributorAuthor
Thanks @melonora, pre-approving. I kindly ask you just two minor adjustments:
  •  please add the deprecation warning for vmin, vmax also in the other 3 render functions.
  •  please modify the warning saying to pass the Normalize object to norm, otherwise it may not be clear how to use it.
    
After this feel free to merge thanks.
added todo as well to add this to tutorial notebook.
@melonora
melonora
merged commit 6cce44e
into
scverse:mainOct 10, 2024
@melonora
melonora
deleted the

issue_#328

branch
October 10, 2024 11:31
@timtreistimtreis
mentioned this pull request
May 8, 2026
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@LucaMarconatoLucaMarconatoLucaMarconato approved these changes
@timtreistimtreisAwaiting requested review from timtreis

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Unable to set vmin vmax when plotting vector data


3 participants

@melonora@codecov-commenter@LucaMarconato

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fix clims when plotting shapes element annotations with matplotlib rendering - #368
Merged
melonora merged 11 commits into
scverse:mainfrom 
melonora:issue_#328
Oct 10, 2024
Merged
Fix clims when plotting shapes element annotations with matplotlib rendering#368
melonora merged 11 commits into
scverse:mainfrom 
melonora:issue_#328
Conversation
@melonora
@melonoramelonora
commented
Oct 3, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
closes#324
This PR fixes the issue described in #324. When creating the patch collection a default Normalize was always created instead of using the one provided by the user. Furthermore in this PR some legacy code was removed which did not do anything.
@melonoramelonora
changed the title
Fix clims when plotting shapes element annotationsFix clims when plotting shapes element annotations with matplotlib renderingOct 3, 2024
@codecov-commenter
codecov-commenter
commented
Oct 4, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 54.54545% with 5 lines in your changes missing coverage. Please review.
Project coverage is 83.73%. Comparing base (6ffe22b) to head (16a5a38).

Report is 1 commits behind head on main.
Files with missing linesPatch %Lines
src/spatialdata_plot/pl/basic.py50.00%4 Missing ⚠️
src/spatialdata_plot/pl/utils.py66.66%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #368 +/- ##
==========================================
- Coverage 83.76% 83.73% -0.04% 
==========================================
Files 8 8 Lines 1540 1543 +3 ==========================================
+ Hits 1290 1292 +2 - Misses 250 251 +1 
Files with missing linesCoverage Δ
src/spatialdata_plot/pl/utils.py76.37% <66.66%> (+0.20%)⬆️
src/spatialdata_plot/pl/basic.py89.26% <50.00%> (-1.60%)⬇️
@melonora
melonora
marked this pull request as ready for review
October 4, 2024 08:36
@melonora
melonora
commented
Oct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
There were some tests that had an expected figure that was wrong in the first place so I fixed that. Think it is good to go for now. Here and there in the test code base I also saw no copies of anndata being created, spamming warnings so I silenced those.
@melonoramelonora
mentioned this pull request
Oct 4, 2024

Closed

LucaMarconato
LucaMarconato
requested changes
Oct 4, 2024
View reviewed changes
Comment threadsrc/spatialdata_plot/pl/utils.py
 elif vcenter is None:
 norm = Normalize(vmin=vmin, vmax=vmax, clip=True)
 else:
 norm = TwoSlopeNorm(vmin=vmin, vmax=vmax, vcenter=vcenter)
@LucaMarconatoLucaMarconatoOct 4, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Now that vcenter is removed, it should be removed also from the function signature. Also, kwargs is in the signature but not used, so I would remove it.
Comment threadsrc/spatialdata_plot/pl/utils.py
 else:
 try:
 norm = colors.Normalize(vmin=min(c), vmax=max(c))
 norm = colors.Normalize(vmin=min(c), vmax=max(c)) if norm is None else norm
@LucaMarconatoLucaMarconatoOct 4, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

In this function Normalize() is initialized without clip, while in _prepare_cmap_norm() the default is to set clip=True. I would choose one of the two as our default choice. The user will be able to specify clip, vcenter, etc by passing a norm object directly.
@melonoramelonoraOct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

hmm let me double check that if we don't pass norm as user, whether ultimately the norm is always created anyway, then we can get rid of normalize instance initiated here.
@melonoramelonoraOct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ok there is code left over of when vmin and vmax were removed. Not certain whether to address this in a different PR.
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'd address the choice of the value of clip in this PR please, because it's easy to forget about this in a new PR.
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

default is set to False now
Comment threadtests/_images/Labels_can_render_given_scale_of_multiscale_labels.png
Comment threadtests/pl/test_render_shapes.py
Comment threadtests/pl/test_render_shapes.py
LucaMarconato
LucaMarconato
reviewed
Oct 9, 2024
View reviewed changes
Comment threadsrc/spatialdata_plot/pl/utils.py
 cmap: Colormap | str | None = None,
 norm: Normalize | None = None,
 na_color: ColorLike | None = None,
 vmin: float | None = None,
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@timtreis I don't remember the outcome of the discussion with the user that reported this. Is this the way to go (=letting users only use norm and not vmin, vmax) or shall we remove vcenter only and keep vmin, vmax and use them to initialize the default Normalize object?
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In the discussion it was stated that vmin and vmax are removed. This function is only internally called
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

So we agree on clip 'True' by default if user does not provide normaloze object?
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If vmin and vmax are not exposed to the user (and hence they are None), then clip will have no effect because when exposed vmin, vmax are None, the data limits are used. So I would keep the default clip to be False (which is matplotlib's default).
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

One thing, if vmin, vmax are removed from pl.render_shapes(), we should throw an informative exception or deprecation warning, explaining to the user that norm should be used instead. Could you add that please?
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

They have not been removed in this PR though and the public functions thus already did not contain these parameters
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I did add a deprecation warning in case of the arguments being passed as kwargs
@LucaMarconato
LucaMarconato
commented
Oct 9, 2024•

edited by melonora

Loading

Copy link


Copy Markdown

Member
Thanks @melonora, pre-approving. I kindly ask you just two minor adjustments:
  •  please add the deprecation warning for vmin, vmax also in the other 3 render functions.
  •  please modify the warning saying to pass the Normalize object to norm, otherwise it may not be clear how to use it.
    
After this feel free to merge thanks.
@melonora
melonora
commented
Oct 10, 2024

Copy link


Copy Markdown

ContributorAuthor
Thanks @melonora, pre-approving. I kindly ask you just two minor adjustments:
  •  please add the deprecation warning for vmin, vmax also in the other 3 render functions.
  •  please modify the warning saying to pass the Normalize object to norm, otherwise it may not be clear how to use it.
    
After this feel free to merge thanks.
added todo as well to add this to tutorial notebook.
@melonora
melonora
merged commit 6cce44e
into
scverse:mainOct 10, 2024
@melonora
melonora
deleted the

issue_#328

branch
October 10, 2024 11:31
@timtreistimtreis
mentioned this pull request
May 8, 2026
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@LucaMarconatoLucaMarconatoLucaMarconato approved these changes
@timtreistimtreisAwaiting requested review from timtreis

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Unable to set vmin vmax when plotting vector data


3 participants

@melonora@codecov-commenter@LucaMarconato

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fix clims when plotting shapes element annotations with matplotlib rendering - #368
Merged
melonora merged 11 commits into
scverse:mainfrom 
melonora:issue_#328
Oct 10, 2024
Merged
Fix clims when plotting shapes element annotations with matplotlib rendering#368
melonora merged 11 commits into
scverse:mainfrom 
melonora:issue_#328
Conversation
@melonora
@melonoramelonora
commented
Oct 3, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
closes#324
This PR fixes the issue described in #324. When creating the patch collection a default Normalize was always created instead of using the one provided by the user. Furthermore in this PR some legacy code was removed which did not do anything.
@melonoramelonora
changed the title
Fix clims when plotting shapes element annotationsFix clims when plotting shapes element annotations with matplotlib renderingOct 3, 2024
@codecov-commenter
codecov-commenter
commented
Oct 4, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 54.54545% with 5 lines in your changes missing coverage. Please review.
Project coverage is 83.73%. Comparing base (6ffe22b) to head (16a5a38).

Report is 1 commits behind head on main.
Files with missing linesPatch %Lines
src/spatialdata_plot/pl/basic.py50.00%4 Missing ⚠️
src/spatialdata_plot/pl/utils.py66.66%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #368 +/- ##
==========================================
- Coverage 83.76% 83.73% -0.04% 
==========================================
Files 8 8 Lines 1540 1543 +3 ==========================================
+ Hits 1290 1292 +2 - Misses 250 251 +1 
Files with missing linesCoverage Δ
src/spatialdata_plot/pl/utils.py76.37% <66.66%> (+0.20%)⬆️
src/spatialdata_plot/pl/basic.py89.26% <50.00%> (-1.60%)⬇️
@melonora
melonora
marked this pull request as ready for review
October 4, 2024 08:36
@melonora
melonora
commented
Oct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
There were some tests that had an expected figure that was wrong in the first place so I fixed that. Think it is good to go for now. Here and there in the test code base I also saw no copies of anndata being created, spamming warnings so I silenced those.
@melonoramelonora
mentioned this pull request
Oct 4, 2024

Closed

LucaMarconato
LucaMarconato
requested changes
Oct 4, 2024
View reviewed changes
Comment threadsrc/spatialdata_plot/pl/utils.py
 elif vcenter is None:
 norm = Normalize(vmin=vmin, vmax=vmax, clip=True)
 else:
 norm = TwoSlopeNorm(vmin=vmin, vmax=vmax, vcenter=vcenter)
@LucaMarconatoLucaMarconatoOct 4, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Now that vcenter is removed, it should be removed also from the function signature. Also, kwargs is in the signature but not used, so I would remove it.
Comment threadsrc/spatialdata_plot/pl/utils.py
 else:
 try:
 norm = colors.Normalize(vmin=min(c), vmax=max(c))
 norm = colors.Normalize(vmin=min(c), vmax=max(c)) if norm is None else norm
@LucaMarconatoLucaMarconatoOct 4, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

In this function Normalize() is initialized without clip, while in _prepare_cmap_norm() the default is to set clip=True. I would choose one of the two as our default choice. The user will be able to specify clip, vcenter, etc by passing a norm object directly.
@melonoramelonoraOct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

hmm let me double check that if we don't pass norm as user, whether ultimately the norm is always created anyway, then we can get rid of normalize instance initiated here.
@melonoramelonoraOct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ok there is code left over of when vmin and vmax were removed. Not certain whether to address this in a different PR.
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'd address the choice of the value of clip in this PR please, because it's easy to forget about this in a new PR.
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

default is set to False now
Comment threadtests/_images/Labels_can_render_given_scale_of_multiscale_labels.png
Comment threadtests/pl/test_render_shapes.py
Comment threadtests/pl/test_render_shapes.py
LucaMarconato
LucaMarconato
reviewed
Oct 9, 2024
View reviewed changes
Comment threadsrc/spatialdata_plot/pl/utils.py
 cmap: Colormap | str | None = None,
 norm: Normalize | None = None,
 na_color: ColorLike | None = None,
 vmin: float | None = None,
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@timtreis I don't remember the outcome of the discussion with the user that reported this. Is this the way to go (=letting users only use norm and not vmin, vmax) or shall we remove vcenter only and keep vmin, vmax and use them to initialize the default Normalize object?
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In the discussion it was stated that vmin and vmax are removed. This function is only internally called
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

So we agree on clip 'True' by default if user does not provide normaloze object?
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If vmin and vmax are not exposed to the user (and hence they are None), then clip will have no effect because when exposed vmin, vmax are None, the data limits are used. So I would keep the default clip to be False (which is matplotlib's default).
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

One thing, if vmin, vmax are removed from pl.render_shapes(), we should throw an informative exception or deprecation warning, explaining to the user that norm should be used instead. Could you add that please?
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

They have not been removed in this PR though and the public functions thus already did not contain these parameters
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I did add a deprecation warning in case of the arguments being passed as kwargs
@LucaMarconato
LucaMarconato
commented
Oct 9, 2024•

edited by melonora

Loading

Copy link


Copy Markdown

Member
Thanks @melonora, pre-approving. I kindly ask you just two minor adjustments:
  •  please add the deprecation warning for vmin, vmax also in the other 3 render functions.
  •  please modify the warning saying to pass the Normalize object to norm, otherwise it may not be clear how to use it.
    
After this feel free to merge thanks.
@melonora
melonora
commented
Oct 10, 2024

Copy link


Copy Markdown

ContributorAuthor
Thanks @melonora, pre-approving. I kindly ask you just two minor adjustments:
  •  please add the deprecation warning for vmin, vmax also in the other 3 render functions.
  •  please modify the warning saying to pass the Normalize object to norm, otherwise it may not be clear how to use it.
    
After this feel free to merge thanks.
added todo as well to add this to tutorial notebook.
@melonora
melonora
merged commit 6cce44e
into
scverse:mainOct 10, 2024
@melonora
melonora
deleted the

issue_#328

branch
October 10, 2024 11:31
@timtreistimtreis
mentioned this pull request
May 8, 2026
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@LucaMarconatoLucaMarconatoLucaMarconato approved these changes
@timtreistimtreisAwaiting requested review from timtreis

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Unable to set vmin vmax when plotting vector data


3 participants

@melonora@codecov-commenter@LucaMarconato

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Fix clims when plotting shapes element annotations with matplotlib rendering - #368
Merged
melonora merged 11 commits into
scverse:mainfrom 
melonora:issue_#328
Oct 10, 2024
Merged
Fix clims when plotting shapes element annotations with matplotlib rendering#368
melonora merged 11 commits into
scverse:mainfrom 
melonora:issue_#328
Conversation
@melonora
@melonoramelonora
commented
Oct 3, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
closes#324
This PR fixes the issue described in #324. When creating the patch collection a default Normalize was always created instead of using the one provided by the user. Furthermore in this PR some legacy code was removed which did not do anything.
@melonoramelonora
changed the title
Fix clims when plotting shapes element annotationsFix clims when plotting shapes element annotations with matplotlib renderingOct 3, 2024
@codecov-commenter
codecov-commenter
commented
Oct 4, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 54.54545% with 5 lines in your changes missing coverage. Please review.
Project coverage is 83.73%. Comparing base (6ffe22b) to head (16a5a38).

Report is 1 commits behind head on main.
Files with missing linesPatch %Lines
src/spatialdata_plot/pl/basic.py50.00%4 Missing ⚠️
src/spatialdata_plot/pl/utils.py66.66%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #368 +/- ##
==========================================
- Coverage 83.76% 83.73% -0.04% 
==========================================
Files 8 8 Lines 1540 1543 +3 ==========================================
+ Hits 1290 1292 +2 - Misses 250 251 +1 
Files with missing linesCoverage Δ
src/spatialdata_plot/pl/utils.py76.37% <66.66%> (+0.20%)⬆️
src/spatialdata_plot/pl/basic.py89.26% <50.00%> (-1.60%)⬇️
@melonora
melonora
marked this pull request as ready for review
October 4, 2024 08:36
@melonora
melonora
commented
Oct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
There were some tests that had an expected figure that was wrong in the first place so I fixed that. Think it is good to go for now. Here and there in the test code base I also saw no copies of anndata being created, spamming warnings so I silenced those.
@melonoramelonora
mentioned this pull request
Oct 4, 2024

Closed

LucaMarconato
LucaMarconato
requested changes
Oct 4, 2024
View reviewed changes
Comment threadsrc/spatialdata_plot/pl/utils.py
 elif vcenter is None:
 norm = Normalize(vmin=vmin, vmax=vmax, clip=True)
 else:
 norm = TwoSlopeNorm(vmin=vmin, vmax=vmax, vcenter=vcenter)
@LucaMarconatoLucaMarconatoOct 4, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Now that vcenter is removed, it should be removed also from the function signature. Also, kwargs is in the signature but not used, so I would remove it.
Comment threadsrc/spatialdata_plot/pl/utils.py
 else:
 try:
 norm = colors.Normalize(vmin=min(c), vmax=max(c))
 norm = colors.Normalize(vmin=min(c), vmax=max(c)) if norm is None else norm
@LucaMarconatoLucaMarconatoOct 4, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

In this function Normalize() is initialized without clip, while in _prepare_cmap_norm() the default is to set clip=True. I would choose one of the two as our default choice. The user will be able to specify clip, vcenter, etc by passing a norm object directly.
@melonoramelonoraOct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

hmm let me double check that if we don't pass norm as user, whether ultimately the norm is always created anyway, then we can get rid of normalize instance initiated here.
@melonoramelonoraOct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ok there is code left over of when vmin and vmax were removed. Not certain whether to address this in a different PR.
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'd address the choice of the value of clip in this PR please, because it's easy to forget about this in a new PR.
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

default is set to False now
Comment threadtests/_images/Labels_can_render_given_scale_of_multiscale_labels.png
Comment threadtests/pl/test_render_shapes.py
Comment threadtests/pl/test_render_shapes.py
LucaMarconato
LucaMarconato
reviewed
Oct 9, 2024
View reviewed changes
Comment threadsrc/spatialdata_plot/pl/utils.py
 cmap: Colormap | str | None = None,
 norm: Normalize | None = None,
 na_color: ColorLike | None = None,
 vmin: float | None = None,
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@timtreis I don't remember the outcome of the discussion with the user that reported this. Is this the way to go (=letting users only use norm and not vmin, vmax) or shall we remove vcenter only and keep vmin, vmax and use them to initialize the default Normalize object?
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In the discussion it was stated that vmin and vmax are removed. This function is only internally called
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

So we agree on clip 'True' by default if user does not provide normaloze object?
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If vmin and vmax are not exposed to the user (and hence they are None), then clip will have no effect because when exposed vmin, vmax are None, the data limits are used. So I would keep the default clip to be False (which is matplotlib's default).
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

One thing, if vmin, vmax are removed from pl.render_shapes(), we should throw an informative exception or deprecation warning, explaining to the user that norm should be used instead. Could you add that please?
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

They have not been removed in this PR though and the public functions thus already did not contain these parameters
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I did add a deprecation warning in case of the arguments being passed as kwargs
@LucaMarconato
LucaMarconato
commented
Oct 9, 2024•

edited by melonora

Loading

Copy link


Copy Markdown

Member
Thanks @melonora, pre-approving. I kindly ask you just two minor adjustments:
  •  please add the deprecation warning for vmin, vmax also in the other 3 render functions.
  •  please modify the warning saying to pass the Normalize object to norm, otherwise it may not be clear how to use it.
    
After this feel free to merge thanks.
@melonora
melonora
commented
Oct 10, 2024

Copy link


Copy Markdown

ContributorAuthor
Thanks @melonora, pre-approving. I kindly ask you just two minor adjustments:
  •  please add the deprecation warning for vmin, vmax also in the other 3 render functions.
  •  please modify the warning saying to pass the Normalize object to norm, otherwise it may not be clear how to use it.
    
After this feel free to merge thanks.
added todo as well to add this to tutorial notebook.
@melonora
melonora
merged commit 6cce44e
into
scverse:mainOct 10, 2024
@melonora
melonora
deleted the

issue_#328

branch
October 10, 2024 11:31
@timtreistimtreis
mentioned this pull request
May 8, 2026
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@LucaMarconatoLucaMarconatoLucaMarconato approved these changes
@timtreistimtreisAwaiting requested review from timtreis

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Unable to set vmin vmax when plotting vector data


3 participants

@melonora@codecov-commenter@LucaMarconato

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fix clims when plotting shapes element annotations with matplotlib rendering - #368
Merged
melonora merged 11 commits into
scverse:mainfrom 
melonora:issue_#328
Oct 10, 2024
Merged
Fix clims when plotting shapes element annotations with matplotlib rendering#368
melonora merged 11 commits into
scverse:mainfrom 
melonora:issue_#328
Conversation
@melonora
@melonoramelonora
commented
Oct 3, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
closes#324
This PR fixes the issue described in #324. When creating the patch collection a default Normalize was always created instead of using the one provided by the user. Furthermore in this PR some legacy code was removed which did not do anything.
@melonoramelonora
changed the title
Fix clims when plotting shapes element annotationsFix clims when plotting shapes element annotations with matplotlib renderingOct 3, 2024
@codecov-commenter
codecov-commenter
commented
Oct 4, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 54.54545% with 5 lines in your changes missing coverage. Please review.
Project coverage is 83.73%. Comparing base (6ffe22b) to head (16a5a38).

Report is 1 commits behind head on main.
Files with missing linesPatch %Lines
src/spatialdata_plot/pl/basic.py50.00%4 Missing ⚠️
src/spatialdata_plot/pl/utils.py66.66%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #368 +/- ##
==========================================
- Coverage 83.76% 83.73% -0.04% 
==========================================
Files 8 8 Lines 1540 1543 +3 ==========================================
+ Hits 1290 1292 +2 - Misses 250 251 +1 
Files with missing linesCoverage Δ
src/spatialdata_plot/pl/utils.py76.37% <66.66%> (+0.20%)⬆️
src/spatialdata_plot/pl/basic.py89.26% <50.00%> (-1.60%)⬇️
@melonora
melonora
marked this pull request as ready for review
October 4, 2024 08:36
@melonora
melonora
commented
Oct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
There were some tests that had an expected figure that was wrong in the first place so I fixed that. Think it is good to go for now. Here and there in the test code base I also saw no copies of anndata being created, spamming warnings so I silenced those.
@melonoramelonora
mentioned this pull request
Oct 4, 2024

Closed

LucaMarconato
LucaMarconato
requested changes
Oct 4, 2024
View reviewed changes
Comment threadsrc/spatialdata_plot/pl/utils.py
 elif vcenter is None:
 norm = Normalize(vmin=vmin, vmax=vmax, clip=True)
 else:
 norm = TwoSlopeNorm(vmin=vmin, vmax=vmax, vcenter=vcenter)
@LucaMarconatoLucaMarconatoOct 4, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Now that vcenter is removed, it should be removed also from the function signature. Also, kwargs is in the signature but not used, so I would remove it.
Comment threadsrc/spatialdata_plot/pl/utils.py
 else:
 try:
 norm = colors.Normalize(vmin=min(c), vmax=max(c))
 norm = colors.Normalize(vmin=min(c), vmax=max(c)) if norm is None else norm
@LucaMarconatoLucaMarconatoOct 4, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

In this function Normalize() is initialized without clip, while in _prepare_cmap_norm() the default is to set clip=True. I would choose one of the two as our default choice. The user will be able to specify clip, vcenter, etc by passing a norm object directly.
@melonoramelonoraOct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

hmm let me double check that if we don't pass norm as user, whether ultimately the norm is always created anyway, then we can get rid of normalize instance initiated here.
@melonoramelonoraOct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ok there is code left over of when vmin and vmax were removed. Not certain whether to address this in a different PR.
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'd address the choice of the value of clip in this PR please, because it's easy to forget about this in a new PR.
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

default is set to False now
Comment threadtests/_images/Labels_can_render_given_scale_of_multiscale_labels.png
Comment threadtests/pl/test_render_shapes.py
Comment threadtests/pl/test_render_shapes.py
LucaMarconato
LucaMarconato
reviewed
Oct 9, 2024
View reviewed changes
Comment threadsrc/spatialdata_plot/pl/utils.py
 cmap: Colormap | str | None = None,
 norm: Normalize | None = None,
 na_color: ColorLike | None = None,
 vmin: float | None = None,
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@timtreis I don't remember the outcome of the discussion with the user that reported this. Is this the way to go (=letting users only use norm and not vmin, vmax) or shall we remove vcenter only and keep vmin, vmax and use them to initialize the default Normalize object?
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In the discussion it was stated that vmin and vmax are removed. This function is only internally called
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

So we agree on clip 'True' by default if user does not provide normaloze object?
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If vmin and vmax are not exposed to the user (and hence they are None), then clip will have no effect because when exposed vmin, vmax are None, the data limits are used. So I would keep the default clip to be False (which is matplotlib's default).
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

One thing, if vmin, vmax are removed from pl.render_shapes(), we should throw an informative exception or deprecation warning, explaining to the user that norm should be used instead. Could you add that please?
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

They have not been removed in this PR though and the public functions thus already did not contain these parameters
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I did add a deprecation warning in case of the arguments being passed as kwargs
@LucaMarconato
LucaMarconato
commented
Oct 9, 2024•

edited by melonora

Loading

Copy link


Copy Markdown

Member
Thanks @melonora, pre-approving. I kindly ask you just two minor adjustments:
  •  please add the deprecation warning for vmin, vmax also in the other 3 render functions.
  •  please modify the warning saying to pass the Normalize object to norm, otherwise it may not be clear how to use it.
    
After this feel free to merge thanks.
@melonora
melonora
commented
Oct 10, 2024

Copy link


Copy Markdown

ContributorAuthor
Thanks @melonora, pre-approving. I kindly ask you just two minor adjustments:
  •  please add the deprecation warning for vmin, vmax also in the other 3 render functions.
  •  please modify the warning saying to pass the Normalize object to norm, otherwise it may not be clear how to use it.
    
After this feel free to merge thanks.
added todo as well to add this to tutorial notebook.
@melonora
melonora
merged commit 6cce44e
into
scverse:mainOct 10, 2024
@melonora
melonora
deleted the

issue_#328

branch
October 10, 2024 11:31
@timtreistimtreis
mentioned this pull request
May 8, 2026
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@LucaMarconatoLucaMarconatoLucaMarconato approved these changes
@timtreistimtreisAwaiting requested review from timtreis

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Unable to set vmin vmax when plotting vector data


3 participants

@melonora@codecov-commenter@LucaMarconato

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fix clims when plotting shapes element annotations with matplotlib rendering - #368
Merged
melonora merged 11 commits into
scverse:mainfrom 
melonora:issue_#328
Oct 10, 2024
Merged
Fix clims when plotting shapes element annotations with matplotlib rendering#368
melonora merged 11 commits into
scverse:mainfrom 
melonora:issue_#328
Conversation
@melonora
@melonoramelonora
commented
Oct 3, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
closes#324
This PR fixes the issue described in #324. When creating the patch collection a default Normalize was always created instead of using the one provided by the user. Furthermore in this PR some legacy code was removed which did not do anything.
@melonoramelonora
changed the title
Fix clims when plotting shapes element annotationsFix clims when plotting shapes element annotations with matplotlib renderingOct 3, 2024
@codecov-commenter
codecov-commenter
commented
Oct 4, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 54.54545% with 5 lines in your changes missing coverage. Please review.
Project coverage is 83.73%. Comparing base (6ffe22b) to head (16a5a38).

Report is 1 commits behind head on main.
Files with missing linesPatch %Lines
src/spatialdata_plot/pl/basic.py50.00%4 Missing ⚠️
src/spatialdata_plot/pl/utils.py66.66%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #368 +/- ##
==========================================
- Coverage 83.76% 83.73% -0.04% 
==========================================
Files 8 8 Lines 1540 1543 +3 ==========================================
+ Hits 1290 1292 +2 - Misses 250 251 +1 
Files with missing linesCoverage Δ
src/spatialdata_plot/pl/utils.py76.37% <66.66%> (+0.20%)⬆️
src/spatialdata_plot/pl/basic.py89.26% <50.00%> (-1.60%)⬇️
@melonora
melonora
marked this pull request as ready for review
October 4, 2024 08:36
@melonora
melonora
commented
Oct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
There were some tests that had an expected figure that was wrong in the first place so I fixed that. Think it is good to go for now. Here and there in the test code base I also saw no copies of anndata being created, spamming warnings so I silenced those.
@melonoramelonora
mentioned this pull request
Oct 4, 2024

Closed

LucaMarconato
LucaMarconato
requested changes
Oct 4, 2024
View reviewed changes
Comment threadsrc/spatialdata_plot/pl/utils.py
 elif vcenter is None:
 norm = Normalize(vmin=vmin, vmax=vmax, clip=True)
 else:
 norm = TwoSlopeNorm(vmin=vmin, vmax=vmax, vcenter=vcenter)
@LucaMarconatoLucaMarconatoOct 4, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Now that vcenter is removed, it should be removed also from the function signature. Also, kwargs is in the signature but not used, so I would remove it.
Comment threadsrc/spatialdata_plot/pl/utils.py
 else:
 try:
 norm = colors.Normalize(vmin=min(c), vmax=max(c))
 norm = colors.Normalize(vmin=min(c), vmax=max(c)) if norm is None else norm
@LucaMarconatoLucaMarconatoOct 4, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

In this function Normalize() is initialized without clip, while in _prepare_cmap_norm() the default is to set clip=True. I would choose one of the two as our default choice. The user will be able to specify clip, vcenter, etc by passing a norm object directly.
@melonoramelonoraOct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

hmm let me double check that if we don't pass norm as user, whether ultimately the norm is always created anyway, then we can get rid of normalize instance initiated here.
@melonoramelonoraOct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ok there is code left over of when vmin and vmax were removed. Not certain whether to address this in a different PR.
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'd address the choice of the value of clip in this PR please, because it's easy to forget about this in a new PR.
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

default is set to False now
Comment threadtests/_images/Labels_can_render_given_scale_of_multiscale_labels.png
Comment threadtests/pl/test_render_shapes.py
Comment threadtests/pl/test_render_shapes.py
LucaMarconato
LucaMarconato
reviewed
Oct 9, 2024
View reviewed changes
Comment threadsrc/spatialdata_plot/pl/utils.py
 cmap: Colormap | str | None = None,
 norm: Normalize | None = None,
 na_color: ColorLike | None = None,
 vmin: float | None = None,
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@timtreis I don't remember the outcome of the discussion with the user that reported this. Is this the way to go (=letting users only use norm and not vmin, vmax) or shall we remove vcenter only and keep vmin, vmax and use them to initialize the default Normalize object?
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In the discussion it was stated that vmin and vmax are removed. This function is only internally called
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

So we agree on clip 'True' by default if user does not provide normaloze object?
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If vmin and vmax are not exposed to the user (and hence they are None), then clip will have no effect because when exposed vmin, vmax are None, the data limits are used. So I would keep the default clip to be False (which is matplotlib's default).
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

One thing, if vmin, vmax are removed from pl.render_shapes(), we should throw an informative exception or deprecation warning, explaining to the user that norm should be used instead. Could you add that please?
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

They have not been removed in this PR though and the public functions thus already did not contain these parameters
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I did add a deprecation warning in case of the arguments being passed as kwargs
@LucaMarconato
LucaMarconato
commented
Oct 9, 2024•

edited by melonora

Loading

Copy link


Copy Markdown

Member
Thanks @melonora, pre-approving. I kindly ask you just two minor adjustments:
  •  please add the deprecation warning for vmin, vmax also in the other 3 render functions.
  •  please modify the warning saying to pass the Normalize object to norm, otherwise it may not be clear how to use it.
    
After this feel free to merge thanks.
@melonora
melonora
commented
Oct 10, 2024

Copy link


Copy Markdown

ContributorAuthor
Thanks @melonora, pre-approving. I kindly ask you just two minor adjustments:
  •  please add the deprecation warning for vmin, vmax also in the other 3 render functions.
  •  please modify the warning saying to pass the Normalize object to norm, otherwise it may not be clear how to use it.
    
After this feel free to merge thanks.
added todo as well to add this to tutorial notebook.
@melonora
melonora
merged commit 6cce44e
into
scverse:mainOct 10, 2024
@melonora
melonora
deleted the

issue_#328

branch
October 10, 2024 11:31
@timtreistimtreis
mentioned this pull request
May 8, 2026
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@LucaMarconatoLucaMarconatoLucaMarconato approved these changes
@timtreistimtreisAwaiting requested review from timtreis

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Unable to set vmin vmax when plotting vector data


3 participants

@melonora@codecov-commenter@LucaMarconato

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Fix clims when plotting shapes element annotations with matplotlib rendering - #368
Merged
melonora merged 11 commits into
scverse:mainfrom 
melonora:issue_#328
Oct 10, 2024
Merged
Fix clims when plotting shapes element annotations with matplotlib rendering#368
melonora merged 11 commits into
scverse:mainfrom 
melonora:issue_#328
Conversation
@melonora
@melonoramelonora
commented
Oct 3, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
closes#324
This PR fixes the issue described in #324. When creating the patch collection a default Normalize was always created instead of using the one provided by the user. Furthermore in this PR some legacy code was removed which did not do anything.
@melonoramelonora
changed the title
Fix clims when plotting shapes element annotationsFix clims when plotting shapes element annotations with matplotlib renderingOct 3, 2024
@codecov-commenter
codecov-commenter
commented
Oct 4, 2024•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Attention: Patch coverage is 54.54545% with 5 lines in your changes missing coverage. Please review.
Project coverage is 83.73%. Comparing base (6ffe22b) to head (16a5a38).

Report is 1 commits behind head on main.
Files with missing linesPatch %Lines
src/spatialdata_plot/pl/basic.py50.00%4 Missing ⚠️
src/spatialdata_plot/pl/utils.py66.66%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #368 +/- ##
==========================================
- Coverage 83.76% 83.73% -0.04% 
==========================================
Files 8 8 Lines 1540 1543 +3 ==========================================
+ Hits 1290 1292 +2 - Misses 250 251 +1 
Files with missing linesCoverage Δ
src/spatialdata_plot/pl/utils.py76.37% <66.66%> (+0.20%)⬆️
src/spatialdata_plot/pl/basic.py89.26% <50.00%> (-1.60%)⬇️
@melonora
melonora
marked this pull request as ready for review
October 4, 2024 08:36
@melonora
melonora
commented
Oct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
There were some tests that had an expected figure that was wrong in the first place so I fixed that. Think it is good to go for now. Here and there in the test code base I also saw no copies of anndata being created, spamming warnings so I silenced those.
@melonoramelonora
mentioned this pull request
Oct 4, 2024

Closed

LucaMarconato
LucaMarconato
requested changes
Oct 4, 2024
View reviewed changes
Comment threadsrc/spatialdata_plot/pl/utils.py
 elif vcenter is None:
 norm = Normalize(vmin=vmin, vmax=vmax, clip=True)
 else:
 norm = TwoSlopeNorm(vmin=vmin, vmax=vmax, vcenter=vcenter)
@LucaMarconatoLucaMarconatoOct 4, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Now that vcenter is removed, it should be removed also from the function signature. Also, kwargs is in the signature but not used, so I would remove it.
Comment threadsrc/spatialdata_plot/pl/utils.py
 else:
 try:
 norm = colors.Normalize(vmin=min(c), vmax=max(c))
 norm = colors.Normalize(vmin=min(c), vmax=max(c)) if norm is None else norm
@LucaMarconatoLucaMarconatoOct 4, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

In this function Normalize() is initialized without clip, while in _prepare_cmap_norm() the default is to set clip=True. I would choose one of the two as our default choice. The user will be able to specify clip, vcenter, etc by passing a norm object directly.
@melonoramelonoraOct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

hmm let me double check that if we don't pass norm as user, whether ultimately the norm is always created anyway, then we can get rid of normalize instance initiated here.
@melonoramelonoraOct 4, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ok there is code left over of when vmin and vmax were removed. Not certain whether to address this in a different PR.
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I'd address the choice of the value of clip in this PR please, because it's easy to forget about this in a new PR.
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

default is set to False now
Comment threadtests/_images/Labels_can_render_given_scale_of_multiscale_labels.png
Comment threadtests/pl/test_render_shapes.py
Comment threadtests/pl/test_render_shapes.py
LucaMarconato
LucaMarconato
reviewed
Oct 9, 2024
View reviewed changes
Comment threadsrc/spatialdata_plot/pl/utils.py
 cmap: Colormap | str | None = None,
 norm: Normalize | None = None,
 na_color: ColorLike | None = None,
 vmin: float | None = None,
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@timtreis I don't remember the outcome of the discussion with the user that reported this. Is this the way to go (=letting users only use norm and not vmin, vmax) or shall we remove vcenter only and keep vmin, vmax and use them to initialize the default Normalize object?
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In the discussion it was stated that vmin and vmax are removed. This function is only internally called
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

So we agree on clip 'True' by default if user does not provide normaloze object?
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

If vmin and vmax are not exposed to the user (and hence they are None), then clip will have no effect because when exposed vmin, vmax are None, the data limits are used. So I would keep the default clip to be False (which is matplotlib's default).
@LucaMarconatoLucaMarconatoOct 9, 2024

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

One thing, if vmin, vmax are removed from pl.render_shapes(), we should throw an informative exception or deprecation warning, explaining to the user that norm should be used instead. Could you add that please?
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

They have not been removed in this PR though and the public functions thus already did not contain these parameters
@melonoramelonoraOct 9, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I did add a deprecation warning in case of the arguments being passed as kwargs
@LucaMarconato
LucaMarconato
commented
Oct 9, 2024•

edited by melonora

Loading

Copy link


Copy Markdown

Member
Thanks @melonora, pre-approving. I kindly ask you just two minor adjustments:
  •  please add the deprecation warning for vmin, vmax also in the other 3 render functions.
  •  please modify the warning saying to pass the Normalize object to norm, otherwise it may not be clear how to use it.
    
After this feel free to merge thanks.
@melonora
melonora
commented
Oct 10, 2024

Copy link


Copy Markdown

ContributorAuthor
Thanks @melonora, pre-approving. I kindly ask you just two minor adjustments:
  •  please add the deprecation warning for vmin, vmax also in the other 3 render functions.
  •  please modify the warning saying to pass the Normalize object to norm, otherwise it may not be clear how to use it.
    
After this feel free to merge thanks.
added todo as well to add this to tutorial notebook.
@melonora
melonora
merged commit 6cce44e
into
scverse:mainOct 10, 2024
@melonora
melonora
deleted the

issue_#328

branch
October 10, 2024 11:31
@timtreistimtreis
mentioned this pull request
May 8, 2026
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@LucaMarconatoLucaMarconatoLucaMarconato approved these changes
@timtreistimtreisAwaiting requested review from timtreis

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Unable to set vmin vmax when plotting vector data


3 participants

@melonora@codecov-commenter@LucaMarconato