Skip to content

fix categories import - #1010

Merged
LucaMarconato merged 5 commits into
scverse:mainfrom
jonas2612:main
Jan 3, 2026
Merged

fix categories import#1010
LucaMarconato merged 5 commits into
scverse:mainfrom
jonas2612:main

Conversation

@jonas2612

Copy link
Copy Markdown
Contributor

Possible solution to Issue #1009

@codecov

codecovBot commented Oct 30, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.20%. Comparing base (84db22b) to head (040021b).

Additional details and impacted files
@@ Coverage Diff @@## main #1010 +/- ##
=======================================
Coverage 92.20% 92.20% =======================================
Files 49 49 Lines 7560 7560 =======================================
Hits 6971 6971 Misses 589 589 
Files with missing linesCoverage Δ
src/spatialdata/models/models.py88.61% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@melonora

melonora commented Oct 30, 2025

Copy link
Copy Markdown
Collaborator

Hi @jonas2612, Thanks for the PR. However, before I look further into this, could you please check whether the problem persists when you use the PR that unpins dask, #1006? I had a different issue with partitioning, but it could be connected so want to see if it would be fixed.

Also, in general, cat.as_known in the newer dask versions does not necessarily preserve the order anymore, so it might be better to wait with this PR a tiny bit as we expect to have the unpinning dask PR reviewed soon. It would then be better to branch of from that point given what I just mentioned.

@jonas2612

Copy link
Copy Markdown
ContributorAuthor

Hi @melonora. Yes, of course. I'll check it today and get back to you

@melonora

Copy link
Copy Markdown
Collaborator

Cheers, please also check this https://github.com/melonora/spatialdata/blob/e017ca7d6107623750196606a07fe8e4407c242f/src/spatialdata/_core/operations/rasterize.py#L674-L677.

This might provide some context for what I mentioned in my message above.

@jonas2612

Copy link
Copy Markdown
ContributorAuthor

That indeed looks very similar to the bug I get.
Still, I checked it with your PR too, but the error still exists:
image
Here, I expect either 500 or 550 genes, but only receive 17.

I understand the difficulty, although do not understand the background well, why the ordering of the categories is important. But if it's better for you, I can branch off and redo the change at a later time point after the PR is reviewed

@melonora

Copy link
Copy Markdown
Collaborator

@jonas2612, the reason why it is important is because as_known is basically reassigning a column. If the order is changed, a particular category belonging to a certain index can have changed.

@LucaMarconato

LucaMarconato commented Jan 3, 2026

Copy link
Copy Markdown
Member

Thanks @jonas2612 for the PR, I will merge it now. It introduces a performance regression but we can address it later.

I did some quick tests and it seems that

data[c] =data[c].cat.as_known()

takes the same time as

data[c] =data[c].cat.set_categories(data[c].compute().cat.categories)

Since there may be some concern around the order of categories with as_known() (@melonora do you have updates from the Dask team?), let's use compute() for now. I opened an issue to remember to explore faster alternatives: #1042.

@LucaMarconato

Copy link
Copy Markdown
Member

I added a test, which fails before the new change in the pair. As I explain in the test, compute() does not preserve the order of categories (same as as_known()). We may want to just accept it as the new behavior, or follow up with the dask team.

@LucaMarconato

Copy link
Copy Markdown
Member

I'll skip the CI from the latest push since I just removed a comment.

@LucaMarconato
LucaMarconato merged commit 8022f5c into scverse:mainJan 3, 2026
1 of 7 checks passed
@jonas2612

Copy link
Copy Markdown
ContributorAuthor

Hi Luca, sorry for the delayed response. Thank you for merging my solution. I hope that the performance didn't suffer too much with that change.

@LucaMarconato

Copy link
Copy Markdown
Member

Thanks for the fix! Actually the performance was impacted quite a lot but I took the opportunity (in a follow up) to add a clear warning message to the user that there should be no needed to actually compute the categories, and inviting to convert that column to categorical before calling the parser: https://github.com/scverse/spatialdata/pull/1061/changes.

Your code is then used as a fallback.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Categorical column in points element becomes filled with NaNs Categories missing with highly partitioned dask dataframes in PointsModel

3 participants

@jonas2612@melonora@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" + '
fix categories import by jonas2612 · Pull Request #1010 · scverse/spatialdata · GitHub
Skip to content

fix categories import - #1010

Merged
LucaMarconato merged 5 commits into
scverse:mainfrom
jonas2612:main
Jan 3, 2026
Merged

fix categories import#1010
LucaMarconato merged 5 commits into
scverse:mainfrom
jonas2612:main

Conversation

@jonas2612

Copy link
Copy Markdown
Contributor

Possible solution to Issue #1009

@codecov

codecovBot commented Oct 30, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.20%. Comparing base (84db22b) to head (040021b).

Additional details and impacted files
@@ Coverage Diff @@## main #1010 +/- ##
=======================================
Coverage 92.20% 92.20% =======================================
Files 49 49 Lines 7560 7560 =======================================
Hits 6971 6971 Misses 589 589 
Files with missing linesCoverage Δ
src/spatialdata/models/models.py88.61% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@melonora

melonora commented Oct 30, 2025

Copy link
Copy Markdown
Collaborator

Hi @jonas2612, Thanks for the PR. However, before I look further into this, could you please check whether the problem persists when you use the PR that unpins dask, #1006? I had a different issue with partitioning, but it could be connected so want to see if it would be fixed.

Also, in general, cat.as_known in the newer dask versions does not necessarily preserve the order anymore, so it might be better to wait with this PR a tiny bit as we expect to have the unpinning dask PR reviewed soon. It would then be better to branch of from that point given what I just mentioned.

@jonas2612

Copy link
Copy Markdown
ContributorAuthor

Hi @melonora. Yes, of course. I'll check it today and get back to you

@melonora

Copy link
Copy Markdown
Collaborator

Cheers, please also check this https://github.com/melonora/spatialdata/blob/e017ca7d6107623750196606a07fe8e4407c242f/src/spatialdata/_core/operations/rasterize.py#L674-L677.

This might provide some context for what I mentioned in my message above.

@jonas2612

Copy link
Copy Markdown
ContributorAuthor

That indeed looks very similar to the bug I get.
Still, I checked it with your PR too, but the error still exists:
image
Here, I expect either 500 or 550 genes, but only receive 17.

I understand the difficulty, although do not understand the background well, why the ordering of the categories is important. But if it's better for you, I can branch off and redo the change at a later time point after the PR is reviewed

@melonora

Copy link
Copy Markdown
Collaborator

@jonas2612, the reason why it is important is because as_known is basically reassigning a column. If the order is changed, a particular category belonging to a certain index can have changed.

@LucaMarconato

LucaMarconato commented Jan 3, 2026

Copy link
Copy Markdown
Member

Thanks @jonas2612 for the PR, I will merge it now. It introduces a performance regression but we can address it later.

I did some quick tests and it seems that

data[c] =data[c].cat.as_known()

takes the same time as

data[c] =data[c].cat.set_categories(data[c].compute().cat.categories)

Since there may be some concern around the order of categories with as_known() (@melonora do you have updates from the Dask team?), let's use compute() for now. I opened an issue to remember to explore faster alternatives: #1042.

@LucaMarconato

Copy link
Copy Markdown
Member

I added a test, which fails before the new change in the pair. As I explain in the test, compute() does not preserve the order of categories (same as as_known()). We may want to just accept it as the new behavior, or follow up with the dask team.

@LucaMarconato

Copy link
Copy Markdown
Member

I'll skip the CI from the latest push since I just removed a comment.

@LucaMarconato
LucaMarconato merged commit 8022f5c into scverse:mainJan 3, 2026
1 of 7 checks passed
@jonas2612

Copy link
Copy Markdown
ContributorAuthor

Hi Luca, sorry for the delayed response. Thank you for merging my solution. I hope that the performance didn't suffer too much with that change.

@LucaMarconato

Copy link
Copy Markdown
Member

Thanks for the fix! Actually the performance was impacted quite a lot but I took the opportunity (in a follow up) to add a clear warning message to the user that there should be no needed to actually compute the categories, and inviting to convert that column to categorical before calling the parser: https://github.com/scverse/spatialdata/pull/1061/changes.

Your code is then used as a fallback.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Categorical column in points element becomes filled with NaNs Categories missing with highly partitioned dask dataframes in PointsModel

3 participants

@jonas2612@melonora@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('^' + ".*" + ' fix categories import by jonas2612 · Pull Request #1010 · scverse/spatialdata · GitHub
Skip to content

fix categories import - #1010

Merged
LucaMarconato merged 5 commits into
scverse:mainfrom
jonas2612:main
Jan 3, 2026
Merged

fix categories import#1010
LucaMarconato merged 5 commits into
scverse:mainfrom
jonas2612:main

Conversation

@jonas2612

Copy link
Copy Markdown
Contributor

Possible solution to Issue #1009

@codecov

codecovBot commented Oct 30, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.20%. Comparing base (84db22b) to head (040021b).

Additional details and impacted files
@@ Coverage Diff @@## main #1010 +/- ##
=======================================
Coverage 92.20% 92.20% =======================================
Files 49 49 Lines 7560 7560 =======================================
Hits 6971 6971 Misses 589 589 
Files with missing linesCoverage Δ
src/spatialdata/models/models.py88.61% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@melonora

melonora commented Oct 30, 2025

Copy link
Copy Markdown
Collaborator

Hi @jonas2612, Thanks for the PR. However, before I look further into this, could you please check whether the problem persists when you use the PR that unpins dask, #1006? I had a different issue with partitioning, but it could be connected so want to see if it would be fixed.

Also, in general, cat.as_known in the newer dask versions does not necessarily preserve the order anymore, so it might be better to wait with this PR a tiny bit as we expect to have the unpinning dask PR reviewed soon. It would then be better to branch of from that point given what I just mentioned.

@jonas2612

Copy link
Copy Markdown
ContributorAuthor

Hi @melonora. Yes, of course. I'll check it today and get back to you

@melonora

Copy link
Copy Markdown
Collaborator

Cheers, please also check this https://github.com/melonora/spatialdata/blob/e017ca7d6107623750196606a07fe8e4407c242f/src/spatialdata/_core/operations/rasterize.py#L674-L677.

This might provide some context for what I mentioned in my message above.

@jonas2612

Copy link
Copy Markdown
ContributorAuthor

That indeed looks very similar to the bug I get.
Still, I checked it with your PR too, but the error still exists:
image
Here, I expect either 500 or 550 genes, but only receive 17.

I understand the difficulty, although do not understand the background well, why the ordering of the categories is important. But if it's better for you, I can branch off and redo the change at a later time point after the PR is reviewed

@melonora

Copy link
Copy Markdown
Collaborator

@jonas2612, the reason why it is important is because as_known is basically reassigning a column. If the order is changed, a particular category belonging to a certain index can have changed.

@LucaMarconato

LucaMarconato commented Jan 3, 2026

Copy link
Copy Markdown
Member

Thanks @jonas2612 for the PR, I will merge it now. It introduces a performance regression but we can address it later.

I did some quick tests and it seems that

data[c] =data[c].cat.as_known()

takes the same time as

data[c] =data[c].cat.set_categories(data[c].compute().cat.categories)

Since there may be some concern around the order of categories with as_known() (@melonora do you have updates from the Dask team?), let's use compute() for now. I opened an issue to remember to explore faster alternatives: #1042.

@LucaMarconato

Copy link
Copy Markdown
Member

I added a test, which fails before the new change in the pair. As I explain in the test, compute() does not preserve the order of categories (same as as_known()). We may want to just accept it as the new behavior, or follow up with the dask team.

@LucaMarconato

Copy link
Copy Markdown
Member

I'll skip the CI from the latest push since I just removed a comment.

@LucaMarconato
LucaMarconato merged commit 8022f5c into scverse:mainJan 3, 2026
1 of 7 checks passed
@jonas2612

Copy link
Copy Markdown
ContributorAuthor

Hi Luca, sorry for the delayed response. Thank you for merging my solution. I hope that the performance didn't suffer too much with that change.

@LucaMarconato

Copy link
Copy Markdown
Member

Thanks for the fix! Actually the performance was impacted quite a lot but I took the opportunity (in a follow up) to add a clear warning message to the user that there should be no needed to actually compute the categories, and inviting to convert that column to categorical before calling the parser: https://github.com/scverse/spatialdata/pull/1061/changes.

Your code is then used as a fallback.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Categorical column in points element becomes filled with NaNs Categories missing with highly partitioned dask dataframes in PointsModel

3 participants

@jonas2612@melonora@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('^' + ".*" + ' fix categories import by jonas2612 · Pull Request #1010 · scverse/spatialdata · GitHub
Skip to content

fix categories import - #1010

Merged
LucaMarconato merged 5 commits into
scverse:mainfrom
jonas2612:main
Jan 3, 2026
Merged

fix categories import#1010
LucaMarconato merged 5 commits into
scverse:mainfrom
jonas2612:main

Conversation

@jonas2612

Copy link
Copy Markdown
Contributor

Possible solution to Issue #1009

@codecov

codecovBot commented Oct 30, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.20%. Comparing base (84db22b) to head (040021b).

Additional details and impacted files
@@ Coverage Diff @@## main #1010 +/- ##
=======================================
Coverage 92.20% 92.20% =======================================
Files 49 49 Lines 7560 7560 =======================================
Hits 6971 6971 Misses 589 589 
Files with missing linesCoverage Δ
src/spatialdata/models/models.py88.61% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@melonora

melonora commented Oct 30, 2025

Copy link
Copy Markdown
Collaborator

Hi @jonas2612, Thanks for the PR. However, before I look further into this, could you please check whether the problem persists when you use the PR that unpins dask, #1006? I had a different issue with partitioning, but it could be connected so want to see if it would be fixed.

Also, in general, cat.as_known in the newer dask versions does not necessarily preserve the order anymore, so it might be better to wait with this PR a tiny bit as we expect to have the unpinning dask PR reviewed soon. It would then be better to branch of from that point given what I just mentioned.

@jonas2612

Copy link
Copy Markdown
ContributorAuthor

Hi @melonora. Yes, of course. I'll check it today and get back to you

@melonora

Copy link
Copy Markdown
Collaborator

Cheers, please also check this https://github.com/melonora/spatialdata/blob/e017ca7d6107623750196606a07fe8e4407c242f/src/spatialdata/_core/operations/rasterize.py#L674-L677.

This might provide some context for what I mentioned in my message above.

@jonas2612

Copy link
Copy Markdown
ContributorAuthor

That indeed looks very similar to the bug I get.
Still, I checked it with your PR too, but the error still exists:
image
Here, I expect either 500 or 550 genes, but only receive 17.

I understand the difficulty, although do not understand the background well, why the ordering of the categories is important. But if it's better for you, I can branch off and redo the change at a later time point after the PR is reviewed

@melonora

Copy link
Copy Markdown
Collaborator

@jonas2612, the reason why it is important is because as_known is basically reassigning a column. If the order is changed, a particular category belonging to a certain index can have changed.

@LucaMarconato

LucaMarconato commented Jan 3, 2026

Copy link
Copy Markdown
Member

Thanks @jonas2612 for the PR, I will merge it now. It introduces a performance regression but we can address it later.

I did some quick tests and it seems that

data[c] =data[c].cat.as_known()

takes the same time as

data[c] =data[c].cat.set_categories(data[c].compute().cat.categories)

Since there may be some concern around the order of categories with as_known() (@melonora do you have updates from the Dask team?), let's use compute() for now. I opened an issue to remember to explore faster alternatives: #1042.

@LucaMarconato

Copy link
Copy Markdown
Member

I added a test, which fails before the new change in the pair. As I explain in the test, compute() does not preserve the order of categories (same as as_known()). We may want to just accept it as the new behavior, or follow up with the dask team.

@LucaMarconato

Copy link
Copy Markdown
Member

I'll skip the CI from the latest push since I just removed a comment.

@LucaMarconato
LucaMarconato merged commit 8022f5c into scverse:mainJan 3, 2026
1 of 7 checks passed
@jonas2612

Copy link
Copy Markdown
ContributorAuthor

Hi Luca, sorry for the delayed response. Thank you for merging my solution. I hope that the performance didn't suffer too much with that change.

@LucaMarconato

Copy link
Copy Markdown
Member

Thanks for the fix! Actually the performance was impacted quite a lot but I took the opportunity (in a follow up) to add a clear warning message to the user that there should be no needed to actually compute the categories, and inviting to convert that column to categorical before calling the parser: https://github.com/scverse/spatialdata/pull/1061/changes.

Your code is then used as a fallback.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Categorical column in points element becomes filled with NaNs Categories missing with highly partitioned dask dataframes in PointsModel

3 participants

@jonas2612@melonora@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" + ' fix categories import by jonas2612 · Pull Request #1010 · scverse/spatialdata · GitHub
Skip to content

fix categories import - #1010

Merged
LucaMarconato merged 5 commits into
scverse:mainfrom
jonas2612:main
Jan 3, 2026
Merged

fix categories import#1010
LucaMarconato merged 5 commits into
scverse:mainfrom
jonas2612:main

Conversation

@jonas2612

Copy link
Copy Markdown
Contributor

Possible solution to Issue #1009

@codecov

codecovBot commented Oct 30, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.20%. Comparing base (84db22b) to head (040021b).

Additional details and impacted files
@@ Coverage Diff @@## main #1010 +/- ##
=======================================
Coverage 92.20% 92.20% =======================================
Files 49 49 Lines 7560 7560 =======================================
Hits 6971 6971 Misses 589 589 
Files with missing linesCoverage Δ
src/spatialdata/models/models.py88.61% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@melonora

melonora commented Oct 30, 2025

Copy link
Copy Markdown
Collaborator

Hi @jonas2612, Thanks for the PR. However, before I look further into this, could you please check whether the problem persists when you use the PR that unpins dask, #1006? I had a different issue with partitioning, but it could be connected so want to see if it would be fixed.

Also, in general, cat.as_known in the newer dask versions does not necessarily preserve the order anymore, so it might be better to wait with this PR a tiny bit as we expect to have the unpinning dask PR reviewed soon. It would then be better to branch of from that point given what I just mentioned.

@jonas2612

Copy link
Copy Markdown
ContributorAuthor

Hi @melonora. Yes, of course. I'll check it today and get back to you

@melonora

Copy link
Copy Markdown
Collaborator

Cheers, please also check this https://github.com/melonora/spatialdata/blob/e017ca7d6107623750196606a07fe8e4407c242f/src/spatialdata/_core/operations/rasterize.py#L674-L677.

This might provide some context for what I mentioned in my message above.

@jonas2612

Copy link
Copy Markdown
ContributorAuthor

That indeed looks very similar to the bug I get.
Still, I checked it with your PR too, but the error still exists:
image
Here, I expect either 500 or 550 genes, but only receive 17.

I understand the difficulty, although do not understand the background well, why the ordering of the categories is important. But if it's better for you, I can branch off and redo the change at a later time point after the PR is reviewed

@melonora

Copy link
Copy Markdown
Collaborator

@jonas2612, the reason why it is important is because as_known is basically reassigning a column. If the order is changed, a particular category belonging to a certain index can have changed.

@LucaMarconato

LucaMarconato commented Jan 3, 2026

Copy link
Copy Markdown
Member

Thanks @jonas2612 for the PR, I will merge it now. It introduces a performance regression but we can address it later.

I did some quick tests and it seems that

data[c] =data[c].cat.as_known()

takes the same time as

data[c] =data[c].cat.set_categories(data[c].compute().cat.categories)

Since there may be some concern around the order of categories with as_known() (@melonora do you have updates from the Dask team?), let's use compute() for now. I opened an issue to remember to explore faster alternatives: #1042.

@LucaMarconato

Copy link
Copy Markdown
Member

I added a test, which fails before the new change in the pair. As I explain in the test, compute() does not preserve the order of categories (same as as_known()). We may want to just accept it as the new behavior, or follow up with the dask team.

@LucaMarconato

Copy link
Copy Markdown
Member

I'll skip the CI from the latest push since I just removed a comment.

@LucaMarconato
LucaMarconato merged commit 8022f5c into scverse:mainJan 3, 2026
1 of 7 checks passed
@jonas2612

Copy link
Copy Markdown
ContributorAuthor

Hi Luca, sorry for the delayed response. Thank you for merging my solution. I hope that the performance didn't suffer too much with that change.

@LucaMarconato

Copy link
Copy Markdown
Member

Thanks for the fix! Actually the performance was impacted quite a lot but I took the opportunity (in a follow up) to add a clear warning message to the user that there should be no needed to actually compute the categories, and inviting to convert that column to categorical before calling the parser: https://github.com/scverse/spatialdata/pull/1061/changes.

Your code is then used as a fallback.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Categorical column in points element becomes filled with NaNs Categories missing with highly partitioned dask dataframes in PointsModel

3 participants

@jonas2612@melonora@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('^' + ".*" + ' fix categories import by jonas2612 · Pull Request #1010 · scverse/spatialdata · GitHub
Skip to content

fix categories import - #1010

Merged
LucaMarconato merged 5 commits into
scverse:mainfrom
jonas2612:main
Jan 3, 2026
Merged

fix categories import#1010
LucaMarconato merged 5 commits into
scverse:mainfrom
jonas2612:main

Conversation

@jonas2612

Copy link
Copy Markdown
Contributor

Possible solution to Issue #1009

@codecov

codecovBot commented Oct 30, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.20%. Comparing base (84db22b) to head (040021b).

Additional details and impacted files
@@ Coverage Diff @@## main #1010 +/- ##
=======================================
Coverage 92.20% 92.20% =======================================
Files 49 49 Lines 7560 7560 =======================================
Hits 6971 6971 Misses 589 589 
Files with missing linesCoverage Δ
src/spatialdata/models/models.py88.61% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@melonora

melonora commented Oct 30, 2025

Copy link
Copy Markdown
Collaborator

Hi @jonas2612, Thanks for the PR. However, before I look further into this, could you please check whether the problem persists when you use the PR that unpins dask, #1006? I had a different issue with partitioning, but it could be connected so want to see if it would be fixed.

Also, in general, cat.as_known in the newer dask versions does not necessarily preserve the order anymore, so it might be better to wait with this PR a tiny bit as we expect to have the unpinning dask PR reviewed soon. It would then be better to branch of from that point given what I just mentioned.

@jonas2612

Copy link
Copy Markdown
ContributorAuthor

Hi @melonora. Yes, of course. I'll check it today and get back to you

@melonora

Copy link
Copy Markdown
Collaborator

Cheers, please also check this https://github.com/melonora/spatialdata/blob/e017ca7d6107623750196606a07fe8e4407c242f/src/spatialdata/_core/operations/rasterize.py#L674-L677.

This might provide some context for what I mentioned in my message above.

@jonas2612

Copy link
Copy Markdown
ContributorAuthor

That indeed looks very similar to the bug I get.
Still, I checked it with your PR too, but the error still exists:
image
Here, I expect either 500 or 550 genes, but only receive 17.

I understand the difficulty, although do not understand the background well, why the ordering of the categories is important. But if it's better for you, I can branch off and redo the change at a later time point after the PR is reviewed

@melonora

Copy link
Copy Markdown
Collaborator

@jonas2612, the reason why it is important is because as_known is basically reassigning a column. If the order is changed, a particular category belonging to a certain index can have changed.

@LucaMarconato

LucaMarconato commented Jan 3, 2026

Copy link
Copy Markdown
Member

Thanks @jonas2612 for the PR, I will merge it now. It introduces a performance regression but we can address it later.

I did some quick tests and it seems that

data[c] =data[c].cat.as_known()

takes the same time as

data[c] =data[c].cat.set_categories(data[c].compute().cat.categories)

Since there may be some concern around the order of categories with as_known() (@melonora do you have updates from the Dask team?), let's use compute() for now. I opened an issue to remember to explore faster alternatives: #1042.

@LucaMarconato

Copy link
Copy Markdown
Member

I added a test, which fails before the new change in the pair. As I explain in the test, compute() does not preserve the order of categories (same as as_known()). We may want to just accept it as the new behavior, or follow up with the dask team.

@LucaMarconato

Copy link
Copy Markdown
Member

I'll skip the CI from the latest push since I just removed a comment.

@LucaMarconato
LucaMarconato merged commit 8022f5c into scverse:mainJan 3, 2026
1 of 7 checks passed
@jonas2612

Copy link
Copy Markdown
ContributorAuthor

Hi Luca, sorry for the delayed response. Thank you for merging my solution. I hope that the performance didn't suffer too much with that change.

@LucaMarconato

Copy link
Copy Markdown
Member

Thanks for the fix! Actually the performance was impacted quite a lot but I took the opportunity (in a follow up) to add a clear warning message to the user that there should be no needed to actually compute the categories, and inviting to convert that column to categorical before calling the parser: https://github.com/scverse/spatialdata/pull/1061/changes.

Your code is then used as a fallback.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Categorical column in points element becomes filled with NaNs Categories missing with highly partitioned dask dataframes in PointsModel

3 participants

@jonas2612@melonora@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('^' + ".*" + ' fix categories import by jonas2612 · Pull Request #1010 · scverse/spatialdata · GitHub
Skip to content

fix categories import - #1010

Merged
LucaMarconato merged 5 commits into
scverse:mainfrom
jonas2612:main
Jan 3, 2026
Merged

fix categories import#1010
LucaMarconato merged 5 commits into
scverse:mainfrom
jonas2612:main

Conversation

@jonas2612

Copy link
Copy Markdown
Contributor

Possible solution to Issue #1009

@codecov

codecovBot commented Oct 30, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.20%. Comparing base (84db22b) to head (040021b).

Additional details and impacted files
@@ Coverage Diff @@## main #1010 +/- ##
=======================================
Coverage 92.20% 92.20% =======================================
Files 49 49 Lines 7560 7560 =======================================
Hits 6971 6971 Misses 589 589 
Files with missing linesCoverage Δ
src/spatialdata/models/models.py88.61% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@melonora

melonora commented Oct 30, 2025

Copy link
Copy Markdown
Collaborator

Hi @jonas2612, Thanks for the PR. However, before I look further into this, could you please check whether the problem persists when you use the PR that unpins dask, #1006? I had a different issue with partitioning, but it could be connected so want to see if it would be fixed.

Also, in general, cat.as_known in the newer dask versions does not necessarily preserve the order anymore, so it might be better to wait with this PR a tiny bit as we expect to have the unpinning dask PR reviewed soon. It would then be better to branch of from that point given what I just mentioned.

@jonas2612

Copy link
Copy Markdown
ContributorAuthor

Hi @melonora. Yes, of course. I'll check it today and get back to you

@melonora

Copy link
Copy Markdown
Collaborator

Cheers, please also check this https://github.com/melonora/spatialdata/blob/e017ca7d6107623750196606a07fe8e4407c242f/src/spatialdata/_core/operations/rasterize.py#L674-L677.

This might provide some context for what I mentioned in my message above.

@jonas2612

Copy link
Copy Markdown
ContributorAuthor

That indeed looks very similar to the bug I get.
Still, I checked it with your PR too, but the error still exists:
image
Here, I expect either 500 or 550 genes, but only receive 17.

I understand the difficulty, although do not understand the background well, why the ordering of the categories is important. But if it's better for you, I can branch off and redo the change at a later time point after the PR is reviewed

@melonora

Copy link
Copy Markdown
Collaborator

@jonas2612, the reason why it is important is because as_known is basically reassigning a column. If the order is changed, a particular category belonging to a certain index can have changed.

@LucaMarconato

LucaMarconato commented Jan 3, 2026

Copy link
Copy Markdown
Member

Thanks @jonas2612 for the PR, I will merge it now. It introduces a performance regression but we can address it later.

I did some quick tests and it seems that

data[c] =data[c].cat.as_known()

takes the same time as

data[c] =data[c].cat.set_categories(data[c].compute().cat.categories)

Since there may be some concern around the order of categories with as_known() (@melonora do you have updates from the Dask team?), let's use compute() for now. I opened an issue to remember to explore faster alternatives: #1042.

@LucaMarconato

Copy link
Copy Markdown
Member

I added a test, which fails before the new change in the pair. As I explain in the test, compute() does not preserve the order of categories (same as as_known()). We may want to just accept it as the new behavior, or follow up with the dask team.

@LucaMarconato

Copy link
Copy Markdown
Member

I'll skip the CI from the latest push since I just removed a comment.

@LucaMarconato
LucaMarconato merged commit 8022f5c into scverse:mainJan 3, 2026
1 of 7 checks passed
@jonas2612

Copy link
Copy Markdown
ContributorAuthor

Hi Luca, sorry for the delayed response. Thank you for merging my solution. I hope that the performance didn't suffer too much with that change.

@LucaMarconato

Copy link
Copy Markdown
Member

Thanks for the fix! Actually the performance was impacted quite a lot but I took the opportunity (in a follow up) to add a clear warning message to the user that there should be no needed to actually compute the categories, and inviting to convert that column to categorical before calling the parser: https://github.com/scverse/spatialdata/pull/1061/changes.

Your code is then used as a fallback.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Categorical column in points element becomes filled with NaNs Categories missing with highly partitioned dask dataframes in PointsModel

3 participants

@jonas2612@melonora@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); } })(); })(); fix categories import by jonas2612 · Pull Request #1010 · scverse/spatialdata · GitHub
Skip to content

fix categories import - #1010

Merged
LucaMarconato merged 5 commits into
scverse:mainfrom
jonas2612:main
Jan 3, 2026
Merged

fix categories import#1010
LucaMarconato merged 5 commits into
scverse:mainfrom
jonas2612:main

Conversation

@jonas2612

Copy link
Copy Markdown
Contributor

Possible solution to Issue #1009

@codecov

codecovBot commented Oct 30, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.20%. Comparing base (84db22b) to head (040021b).

Additional details and impacted files
@@ Coverage Diff @@## main #1010 +/- ##
=======================================
Coverage 92.20% 92.20% =======================================
Files 49 49 Lines 7560 7560 =======================================
Hits 6971 6971 Misses 589 589 
Files with missing linesCoverage Δ
src/spatialdata/models/models.py88.61% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@melonora

melonora commented Oct 30, 2025

Copy link
Copy Markdown
Collaborator

Hi @jonas2612, Thanks for the PR. However, before I look further into this, could you please check whether the problem persists when you use the PR that unpins dask, #1006? I had a different issue with partitioning, but it could be connected so want to see if it would be fixed.

Also, in general, cat.as_known in the newer dask versions does not necessarily preserve the order anymore, so it might be better to wait with this PR a tiny bit as we expect to have the unpinning dask PR reviewed soon. It would then be better to branch of from that point given what I just mentioned.

@jonas2612

Copy link
Copy Markdown
ContributorAuthor

Hi @melonora. Yes, of course. I'll check it today and get back to you

@melonora

Copy link
Copy Markdown
Collaborator

Cheers, please also check this https://github.com/melonora/spatialdata/blob/e017ca7d6107623750196606a07fe8e4407c242f/src/spatialdata/_core/operations/rasterize.py#L674-L677.

This might provide some context for what I mentioned in my message above.

@jonas2612

Copy link
Copy Markdown
ContributorAuthor

That indeed looks very similar to the bug I get.
Still, I checked it with your PR too, but the error still exists:
image
Here, I expect either 500 or 550 genes, but only receive 17.

I understand the difficulty, although do not understand the background well, why the ordering of the categories is important. But if it's better for you, I can branch off and redo the change at a later time point after the PR is reviewed

@melonora

Copy link
Copy Markdown
Collaborator

@jonas2612, the reason why it is important is because as_known is basically reassigning a column. If the order is changed, a particular category belonging to a certain index can have changed.

@LucaMarconato

LucaMarconato commented Jan 3, 2026

Copy link
Copy Markdown
Member

Thanks @jonas2612 for the PR, I will merge it now. It introduces a performance regression but we can address it later.

I did some quick tests and it seems that

data[c] =data[c].cat.as_known()

takes the same time as

data[c] =data[c].cat.set_categories(data[c].compute().cat.categories)

Since there may be some concern around the order of categories with as_known() (@melonora do you have updates from the Dask team?), let's use compute() for now. I opened an issue to remember to explore faster alternatives: #1042.

@LucaMarconato

Copy link
Copy Markdown
Member

I added a test, which fails before the new change in the pair. As I explain in the test, compute() does not preserve the order of categories (same as as_known()). We may want to just accept it as the new behavior, or follow up with the dask team.

@LucaMarconato

Copy link
Copy Markdown
Member

I'll skip the CI from the latest push since I just removed a comment.

@LucaMarconato
LucaMarconato merged commit 8022f5c into scverse:mainJan 3, 2026
1 of 7 checks passed
@jonas2612

Copy link
Copy Markdown
ContributorAuthor

Hi Luca, sorry for the delayed response. Thank you for merging my solution. I hope that the performance didn't suffer too much with that change.

@LucaMarconato

Copy link
Copy Markdown
Member

Thanks for the fix! Actually the performance was impacted quite a lot but I took the opportunity (in a follow up) to add a clear warning message to the user that there should be no needed to actually compute the categories, and inviting to convert that column to categorical before calling the parser: https://github.com/scverse/spatialdata/pull/1061/changes.

Your code is then used as a fallback.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Categorical column in points element becomes filled with NaNs Categories missing with highly partitioned dask dataframes in PointsModel

3 participants

@jonas2612@melonora@LucaMarconato