Stephen/spaceranger2.0

[stephenwilliams22]

This PR updates scanpy to accommodate the latest changes made in spaceranger 2.0 which will break the released version of scanpy. Will provide backwards compatibility to pre 2.0 releases.

[codecov]

Codecov Report

Merging #2296 (51a9126) into master (0fec570) will increase coverage by 71.49%.
The diff coverage is 100.00%.

❗ Current head 51a9126 differs from pull request most recent head 241ebb5. Consider uploading reports for the commit 241ebb5 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## master #2296 +/- ##
===========================================
+ Coverage 0.31% 71.80% +71.49% 
===========================================
Files 97 97 Lines 11485 11523 +38 ===========================================
+ Hits 36 8274 +8238 + Misses 11449 3249 -8200 

Impacted Files	Coverage Δ
scanpy/readwrite.py	67.74% <100.00%> (+67.74%)	⬆️
scanpy/__init__.py
scanpy/_utils/compute/is_constant.py	70.76% <0.00%> (ø)
scanpy/external/exporting.py	11.86% <0.00%> (+11.86%)	⬆️
scanpy/tools/_louvain.py	20.21% <0.00%> (+20.21%)	⬆️
scanpy/external/tl/_wishbone.py	21.05% <0.00%> (+21.05%)	⬆️
scanpy/external/tl/_palantir.py	22.58% <0.00%> (+22.58%)	⬆️
scanpy/external/tl/_harmony_timeseries.py	25.00% <0.00%> (+25.00%)	⬆️
scanpy/external/tl/_trimap.py	25.92% <0.00%> (+25.92%)	⬆️
scanpy/external/tl/_pypairs.py	26.47% <0.00%> (+26.47%)	⬆️
... and 86 more

[Zethson]

Thank you! Maybe also relevant for @giovp ?

[stephenwilliams22]

@Zethson just wanted to see if there were any issues with merging this PR.

[giovp]

hi @stephenwilliams22 , thanks for this PR, only one minor thing.

[stephenwilliams22]

@giovp looking at this again, it seems you drop the mixed up columns anyway (https://github.com/scverse/squidpy/blob/fb069ded7515e8a1386224d32344a8657cbecd2e/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

[giovp]

@giovp looking at this again, it seems you drop the mixed up columns anyway (scverse/squidpy@fb069de/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

Hi @stephenwilliams22 ,

we drop it from obs since we don't want to save spatial coordinates there, cause they should be only in obsm.

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

[ivirshup]

@giovp, should this change happen in scanpy or squidpy?

[giovp]

@giovp, should this change happen in scanpy or squidpy?

I would make it happen in both and deprecate this function from the next release (as well as all the other spatial).

[ivirshup]

Has this happened in squidpy already?

[giovp]

Has this happened in squidpy already?

yes, need one further clarification from @stephenwilliams22 and then will resolve also in squidpy.

[stephenwilliams22]

@giovp looking at this again, it seems you drop the mixed up columns anyway (scverse/squidpy@fb069de/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

Hi @stephenwilliams22 ,

we drop it from obs since we don't want to save spatial coordinates there, cause they should be only in obsm.

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

Yes, this is correct.

[stephenwilliams22]

@giovp do we have an update on merging this yet? Anything left on my end?

[ivirshup]

@giovp, @stephenwilliams22, @Zethson

This PR seems to have broken a number of tests. I believe due to the change in pixel_row/ pixel_col order, resulting in a number of test plots now being transposed.

If this was always wrong... surely usage would have caught that, so we must be correcting for this somewhere else.

What's the correction here? And is it something that should be done quickly, or should I revert this PR until we can figure it out?

[giovp]

from discussing with @stephenwilliams22 this made me think there shouldn't be changes with column names yet it seems like it is,

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

Yes, this is correct.

we actually might have had similar reports in squidpy as well, let me ask @LLehren for support, will probably have to fix it in squidpy side as well.

[giovp]

for reference scverse/squidpy#599

[ivirshup]

Alright, I'm going to revert for now then

[stephenwilliams22]

This PR updates scanpy to accommodate the latest changes made in spaceranger 2.0 which will break the released version of scanpy. Will provide backwards compatibility to pre 2.0 releases.

[codecov]

Codecov Report

Merging #2296 (51a9126) into master (0fec570) will increase coverage by 71.49%.
The diff coverage is 100.00%.

❗ Current head 51a9126 differs from pull request most recent head 241ebb5. Consider uploading reports for the commit 241ebb5 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## master #2296 +/- ##
===========================================
+ Coverage 0.31% 71.80% +71.49% 
===========================================
Files 97 97 Lines 11485 11523 +38 ===========================================
+ Hits 36 8274 +8238 + Misses 11449 3249 -8200 

Impacted Files	Coverage Δ
scanpy/readwrite.py	67.74% <100.00%> (+67.74%)	⬆️
scanpy/__init__.py
scanpy/_utils/compute/is_constant.py	70.76% <0.00%> (ø)
scanpy/external/exporting.py	11.86% <0.00%> (+11.86%)	⬆️
scanpy/tools/_louvain.py	20.21% <0.00%> (+20.21%)	⬆️
scanpy/external/tl/_wishbone.py	21.05% <0.00%> (+21.05%)	⬆️
scanpy/external/tl/_palantir.py	22.58% <0.00%> (+22.58%)	⬆️
scanpy/external/tl/_harmony_timeseries.py	25.00% <0.00%> (+25.00%)	⬆️
scanpy/external/tl/_trimap.py	25.92% <0.00%> (+25.92%)	⬆️
scanpy/external/tl/_pypairs.py	26.47% <0.00%> (+26.47%)	⬆️
... and 86 more

[Zethson]

Thank you! Maybe also relevant for @giovp ?

[stephenwilliams22]

@Zethson just wanted to see if there were any issues with merging this PR.

[giovp]

hi @stephenwilliams22 , thanks for this PR, only one minor thing.

[stephenwilliams22]

@giovp looking at this again, it seems you drop the mixed up columns anyway (https://github.com/scverse/squidpy/blob/fb069ded7515e8a1386224d32344a8657cbecd2e/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

[giovp]

@giovp looking at this again, it seems you drop the mixed up columns anyway (scverse/squidpy@fb069de/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

Hi @stephenwilliams22 ,

we drop it from obs since we don't want to save spatial coordinates there, cause they should be only in obsm.

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

[ivirshup]

@giovp, should this change happen in scanpy or squidpy?

[giovp]

@giovp, should this change happen in scanpy or squidpy?

I would make it happen in both and deprecate this function from the next release (as well as all the other spatial).

[ivirshup]

Has this happened in squidpy already?

[giovp]

Has this happened in squidpy already?

yes, need one further clarification from @stephenwilliams22 and then will resolve also in squidpy.

[stephenwilliams22]

@giovp looking at this again, it seems you drop the mixed up columns anyway (scverse/squidpy@fb069de/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

Hi @stephenwilliams22 ,

we drop it from obs since we don't want to save spatial coordinates there, cause they should be only in obsm.

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

Yes, this is correct.

[stephenwilliams22]

@giovp do we have an update on merging this yet? Anything left on my end?

[ivirshup]

@giovp, @stephenwilliams22, @Zethson

This PR seems to have broken a number of tests. I believe due to the change in pixel_row/ pixel_col order, resulting in a number of test plots now being transposed.

If this was always wrong... surely usage would have caught that, so we must be correcting for this somewhere else.

What's the correction here? And is it something that should be done quickly, or should I revert this PR until we can figure it out?

[giovp]

from discussing with @stephenwilliams22 this made me think there shouldn't be changes with column names yet it seems like it is,

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

Yes, this is correct.

we actually might have had similar reports in squidpy as well, let me ask @LLehren for support, will probably have to fix it in squidpy side as well.

[giovp]

for reference scverse/squidpy#599

[ivirshup]

Alright, I'm going to revert for now then

[stephenwilliams22]

This PR updates scanpy to accommodate the latest changes made in spaceranger 2.0 which will break the released version of scanpy. Will provide backwards compatibility to pre 2.0 releases.

[codecov]

Codecov Report

Merging #2296 (51a9126) into master (0fec570) will increase coverage by 71.49%.
The diff coverage is 100.00%.

❗ Current head 51a9126 differs from pull request most recent head 241ebb5. Consider uploading reports for the commit 241ebb5 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## master #2296 +/- ##
===========================================
+ Coverage 0.31% 71.80% +71.49% 
===========================================
Files 97 97 Lines 11485 11523 +38 ===========================================
+ Hits 36 8274 +8238 + Misses 11449 3249 -8200 

Impacted Files	Coverage Δ
scanpy/readwrite.py	67.74% <100.00%> (+67.74%)	⬆️
scanpy/__init__.py
scanpy/_utils/compute/is_constant.py	70.76% <0.00%> (ø)
scanpy/external/exporting.py	11.86% <0.00%> (+11.86%)	⬆️
scanpy/tools/_louvain.py	20.21% <0.00%> (+20.21%)	⬆️
scanpy/external/tl/_wishbone.py	21.05% <0.00%> (+21.05%)	⬆️
scanpy/external/tl/_palantir.py	22.58% <0.00%> (+22.58%)	⬆️
scanpy/external/tl/_harmony_timeseries.py	25.00% <0.00%> (+25.00%)	⬆️
scanpy/external/tl/_trimap.py	25.92% <0.00%> (+25.92%)	⬆️
scanpy/external/tl/_pypairs.py	26.47% <0.00%> (+26.47%)	⬆️
... and 86 more

[Zethson]

Thank you! Maybe also relevant for @giovp ?

[stephenwilliams22]

@Zethson just wanted to see if there were any issues with merging this PR.

[giovp]

hi @stephenwilliams22 , thanks for this PR, only one minor thing.

[stephenwilliams22]

@giovp looking at this again, it seems you drop the mixed up columns anyway (https://github.com/scverse/squidpy/blob/fb069ded7515e8a1386224d32344a8657cbecd2e/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

[giovp]

@giovp looking at this again, it seems you drop the mixed up columns anyway (scverse/squidpy@fb069de/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

Hi @stephenwilliams22 ,

we drop it from obs since we don't want to save spatial coordinates there, cause they should be only in obsm.

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

[ivirshup]

@giovp, should this change happen in scanpy or squidpy?

[giovp]

@giovp, should this change happen in scanpy or squidpy?

I would make it happen in both and deprecate this function from the next release (as well as all the other spatial).

[ivirshup]

Has this happened in squidpy already?

[giovp]

Has this happened in squidpy already?

yes, need one further clarification from @stephenwilliams22 and then will resolve also in squidpy.

[stephenwilliams22]

@giovp looking at this again, it seems you drop the mixed up columns anyway (scverse/squidpy@fb069de/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

Hi @stephenwilliams22 ,

we drop it from obs since we don't want to save spatial coordinates there, cause they should be only in obsm.

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

Yes, this is correct.

[stephenwilliams22]

@giovp do we have an update on merging this yet? Anything left on my end?

[ivirshup]

@giovp, @stephenwilliams22, @Zethson

This PR seems to have broken a number of tests. I believe due to the change in pixel_row/ pixel_col order, resulting in a number of test plots now being transposed.

If this was always wrong... surely usage would have caught that, so we must be correcting for this somewhere else.

What's the correction here? And is it something that should be done quickly, or should I revert this PR until we can figure it out?

[giovp]

from discussing with @stephenwilliams22 this made me think there shouldn't be changes with column names yet it seems like it is,

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

Yes, this is correct.

we actually might have had similar reports in squidpy as well, let me ask @LLehren for support, will probably have to fix it in squidpy side as well.

[giovp]

for reference scverse/squidpy#599

[ivirshup]

Alright, I'm going to revert for now then

[stephenwilliams22]

This PR updates scanpy to accommodate the latest changes made in spaceranger 2.0 which will break the released version of scanpy. Will provide backwards compatibility to pre 2.0 releases.

[codecov]

Codecov Report

Merging #2296 (51a9126) into master (0fec570) will increase coverage by 71.49%.
The diff coverage is 100.00%.

❗ Current head 51a9126 differs from pull request most recent head 241ebb5. Consider uploading reports for the commit 241ebb5 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## master #2296 +/- ##
===========================================
+ Coverage 0.31% 71.80% +71.49% 
===========================================
Files 97 97 Lines 11485 11523 +38 ===========================================
+ Hits 36 8274 +8238 + Misses 11449 3249 -8200 

Impacted Files	Coverage Δ
scanpy/readwrite.py	67.74% <100.00%> (+67.74%)	⬆️
scanpy/__init__.py
scanpy/_utils/compute/is_constant.py	70.76% <0.00%> (ø)
scanpy/external/exporting.py	11.86% <0.00%> (+11.86%)	⬆️
scanpy/tools/_louvain.py	20.21% <0.00%> (+20.21%)	⬆️
scanpy/external/tl/_wishbone.py	21.05% <0.00%> (+21.05%)	⬆️
scanpy/external/tl/_palantir.py	22.58% <0.00%> (+22.58%)	⬆️
scanpy/external/tl/_harmony_timeseries.py	25.00% <0.00%> (+25.00%)	⬆️
scanpy/external/tl/_trimap.py	25.92% <0.00%> (+25.92%)	⬆️
scanpy/external/tl/_pypairs.py	26.47% <0.00%> (+26.47%)	⬆️
... and 86 more

[Zethson]

Thank you! Maybe also relevant for @giovp ?

[stephenwilliams22]

@Zethson just wanted to see if there were any issues with merging this PR.

[giovp]

hi @stephenwilliams22 , thanks for this PR, only one minor thing.

[stephenwilliams22]

@giovp looking at this again, it seems you drop the mixed up columns anyway (https://github.com/scverse/squidpy/blob/fb069ded7515e8a1386224d32344a8657cbecd2e/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

[giovp]

@giovp looking at this again, it seems you drop the mixed up columns anyway (scverse/squidpy@fb069de/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

Hi @stephenwilliams22 ,

we drop it from obs since we don't want to save spatial coordinates there, cause they should be only in obsm.

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

[ivirshup]

@giovp, should this change happen in scanpy or squidpy?

[giovp]

@giovp, should this change happen in scanpy or squidpy?

I would make it happen in both and deprecate this function from the next release (as well as all the other spatial).

[ivirshup]

Has this happened in squidpy already?

[giovp]

Has this happened in squidpy already?

yes, need one further clarification from @stephenwilliams22 and then will resolve also in squidpy.

[stephenwilliams22]

@giovp looking at this again, it seems you drop the mixed up columns anyway (scverse/squidpy@fb069de/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

Hi @stephenwilliams22 ,

we drop it from obs since we don't want to save spatial coordinates there, cause they should be only in obsm.

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

Yes, this is correct.

[stephenwilliams22]

@giovp do we have an update on merging this yet? Anything left on my end?

[ivirshup]

@giovp, @stephenwilliams22, @Zethson

This PR seems to have broken a number of tests. I believe due to the change in pixel_row/ pixel_col order, resulting in a number of test plots now being transposed.

If this was always wrong... surely usage would have caught that, so we must be correcting for this somewhere else.

What's the correction here? And is it something that should be done quickly, or should I revert this PR until we can figure it out?

[giovp]

from discussing with @stephenwilliams22 this made me think there shouldn't be changes with column names yet it seems like it is,

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

Yes, this is correct.

we actually might have had similar reports in squidpy as well, let me ask @LLehren for support, will probably have to fix it in squidpy side as well.

[giovp]

for reference scverse/squidpy#599

[ivirshup]

Alright, I'm going to revert for now then

[stephenwilliams22]

This PR updates scanpy to accommodate the latest changes made in spaceranger 2.0 which will break the released version of scanpy. Will provide backwards compatibility to pre 2.0 releases.

[codecov]

Codecov Report

Merging #2296 (51a9126) into master (0fec570) will increase coverage by 71.49%.
The diff coverage is 100.00%.

❗ Current head 51a9126 differs from pull request most recent head 241ebb5. Consider uploading reports for the commit 241ebb5 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## master #2296 +/- ##
===========================================
+ Coverage 0.31% 71.80% +71.49% 
===========================================
Files 97 97 Lines 11485 11523 +38 ===========================================
+ Hits 36 8274 +8238 + Misses 11449 3249 -8200 

Impacted Files	Coverage Δ
scanpy/readwrite.py	67.74% <100.00%> (+67.74%)	⬆️
scanpy/__init__.py
scanpy/_utils/compute/is_constant.py	70.76% <0.00%> (ø)
scanpy/external/exporting.py	11.86% <0.00%> (+11.86%)	⬆️
scanpy/tools/_louvain.py	20.21% <0.00%> (+20.21%)	⬆️
scanpy/external/tl/_wishbone.py	21.05% <0.00%> (+21.05%)	⬆️
scanpy/external/tl/_palantir.py	22.58% <0.00%> (+22.58%)	⬆️
scanpy/external/tl/_harmony_timeseries.py	25.00% <0.00%> (+25.00%)	⬆️
scanpy/external/tl/_trimap.py	25.92% <0.00%> (+25.92%)	⬆️
scanpy/external/tl/_pypairs.py	26.47% <0.00%> (+26.47%)	⬆️
... and 86 more

[Zethson]

Thank you! Maybe also relevant for @giovp ?

[stephenwilliams22]

@Zethson just wanted to see if there were any issues with merging this PR.

[giovp]

hi @stephenwilliams22 , thanks for this PR, only one minor thing.

[stephenwilliams22]

@giovp looking at this again, it seems you drop the mixed up columns anyway (https://github.com/scverse/squidpy/blob/fb069ded7515e8a1386224d32344a8657cbecd2e/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

[giovp]

@giovp looking at this again, it seems you drop the mixed up columns anyway (scverse/squidpy@fb069de/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

Hi @stephenwilliams22 ,

we drop it from obs since we don't want to save spatial coordinates there, cause they should be only in obsm.

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

[ivirshup]

@giovp, should this change happen in scanpy or squidpy?

[giovp]

@giovp, should this change happen in scanpy or squidpy?

I would make it happen in both and deprecate this function from the next release (as well as all the other spatial).

[ivirshup]

Has this happened in squidpy already?

[giovp]

Has this happened in squidpy already?

yes, need one further clarification from @stephenwilliams22 and then will resolve also in squidpy.

[stephenwilliams22]

@giovp looking at this again, it seems you drop the mixed up columns anyway (scverse/squidpy@fb069de/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

Hi @stephenwilliams22 ,

we drop it from obs since we don't want to save spatial coordinates there, cause they should be only in obsm.

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

Yes, this is correct.

[stephenwilliams22]

@giovp do we have an update on merging this yet? Anything left on my end?

[ivirshup]

@giovp, @stephenwilliams22, @Zethson

This PR seems to have broken a number of tests. I believe due to the change in pixel_row/ pixel_col order, resulting in a number of test plots now being transposed.

If this was always wrong... surely usage would have caught that, so we must be correcting for this somewhere else.

What's the correction here? And is it something that should be done quickly, or should I revert this PR until we can figure it out?

[giovp]

from discussing with @stephenwilliams22 this made me think there shouldn't be changes with column names yet it seems like it is,

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

Yes, this is correct.

we actually might have had similar reports in squidpy as well, let me ask @LLehren for support, will probably have to fix it in squidpy side as well.

[giovp]

for reference scverse/squidpy#599

[ivirshup]

Alright, I'm going to revert for now then

[stephenwilliams22]

This PR updates scanpy to accommodate the latest changes made in spaceranger 2.0 which will break the released version of scanpy. Will provide backwards compatibility to pre 2.0 releases.

[codecov]

Codecov Report

Merging #2296 (51a9126) into master (0fec570) will increase coverage by 71.49%.
The diff coverage is 100.00%.

❗ Current head 51a9126 differs from pull request most recent head 241ebb5. Consider uploading reports for the commit 241ebb5 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## master #2296 +/- ##
===========================================
+ Coverage 0.31% 71.80% +71.49% 
===========================================
Files 97 97 Lines 11485 11523 +38 ===========================================
+ Hits 36 8274 +8238 + Misses 11449 3249 -8200 

Impacted Files	Coverage Δ
scanpy/readwrite.py	67.74% <100.00%> (+67.74%)	⬆️
scanpy/__init__.py
scanpy/_utils/compute/is_constant.py	70.76% <0.00%> (ø)
scanpy/external/exporting.py	11.86% <0.00%> (+11.86%)	⬆️
scanpy/tools/_louvain.py	20.21% <0.00%> (+20.21%)	⬆️
scanpy/external/tl/_wishbone.py	21.05% <0.00%> (+21.05%)	⬆️
scanpy/external/tl/_palantir.py	22.58% <0.00%> (+22.58%)	⬆️
scanpy/external/tl/_harmony_timeseries.py	25.00% <0.00%> (+25.00%)	⬆️
scanpy/external/tl/_trimap.py	25.92% <0.00%> (+25.92%)	⬆️
scanpy/external/tl/_pypairs.py	26.47% <0.00%> (+26.47%)	⬆️
... and 86 more

[Zethson]

Thank you! Maybe also relevant for @giovp ?

[stephenwilliams22]

@Zethson just wanted to see if there were any issues with merging this PR.

[giovp]

hi @stephenwilliams22 , thanks for this PR, only one minor thing.

[stephenwilliams22]

@giovp looking at this again, it seems you drop the mixed up columns anyway (https://github.com/scverse/squidpy/blob/fb069ded7515e8a1386224d32344a8657cbecd2e/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

[giovp]

@giovp looking at this again, it seems you drop the mixed up columns anyway (scverse/squidpy@fb069de/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

Hi @stephenwilliams22 ,

we drop it from obs since we don't want to save spatial coordinates there, cause they should be only in obsm.

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

[ivirshup]

@giovp, should this change happen in scanpy or squidpy?

[giovp]

@giovp, should this change happen in scanpy or squidpy?

I would make it happen in both and deprecate this function from the next release (as well as all the other spatial).

[ivirshup]

Has this happened in squidpy already?

[giovp]

Has this happened in squidpy already?

yes, need one further clarification from @stephenwilliams22 and then will resolve also in squidpy.

[stephenwilliams22]

@giovp looking at this again, it seems you drop the mixed up columns anyway (scverse/squidpy@fb069de/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

Hi @stephenwilliams22 ,

we drop it from obs since we don't want to save spatial coordinates there, cause they should be only in obsm.

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

Yes, this is correct.

[stephenwilliams22]

@giovp do we have an update on merging this yet? Anything left on my end?

[ivirshup]

@giovp, @stephenwilliams22, @Zethson

This PR seems to have broken a number of tests. I believe due to the change in pixel_row/ pixel_col order, resulting in a number of test plots now being transposed.

If this was always wrong... surely usage would have caught that, so we must be correcting for this somewhere else.

What's the correction here? And is it something that should be done quickly, or should I revert this PR until we can figure it out?

[giovp]

from discussing with @stephenwilliams22 this made me think there shouldn't be changes with column names yet it seems like it is,

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

Yes, this is correct.

we actually might have had similar reports in squidpy as well, let me ask @LLehren for support, will probably have to fix it in squidpy side as well.

[giovp]

for reference scverse/squidpy#599

[ivirshup]

Alright, I'm going to revert for now then

[stephenwilliams22]

This PR updates scanpy to accommodate the latest changes made in spaceranger 2.0 which will break the released version of scanpy. Will provide backwards compatibility to pre 2.0 releases.

[codecov]

Codecov Report

Merging #2296 (51a9126) into master (0fec570) will increase coverage by 71.49%.
The diff coverage is 100.00%.

❗ Current head 51a9126 differs from pull request most recent head 241ebb5. Consider uploading reports for the commit 241ebb5 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## master #2296 +/- ##
===========================================
+ Coverage 0.31% 71.80% +71.49% 
===========================================
Files 97 97 Lines 11485 11523 +38 ===========================================
+ Hits 36 8274 +8238 + Misses 11449 3249 -8200 

Impacted Files	Coverage Δ
scanpy/readwrite.py	67.74% <100.00%> (+67.74%)	⬆️
scanpy/__init__.py
scanpy/_utils/compute/is_constant.py	70.76% <0.00%> (ø)
scanpy/external/exporting.py	11.86% <0.00%> (+11.86%)	⬆️
scanpy/tools/_louvain.py	20.21% <0.00%> (+20.21%)	⬆️
scanpy/external/tl/_wishbone.py	21.05% <0.00%> (+21.05%)	⬆️
scanpy/external/tl/_palantir.py	22.58% <0.00%> (+22.58%)	⬆️
scanpy/external/tl/_harmony_timeseries.py	25.00% <0.00%> (+25.00%)	⬆️
scanpy/external/tl/_trimap.py	25.92% <0.00%> (+25.92%)	⬆️
scanpy/external/tl/_pypairs.py	26.47% <0.00%> (+26.47%)	⬆️
... and 86 more

[Zethson]

Thank you! Maybe also relevant for @giovp ?

[stephenwilliams22]

@Zethson just wanted to see if there were any issues with merging this PR.

[giovp]

hi @stephenwilliams22 , thanks for this PR, only one minor thing.

[stephenwilliams22]

@giovp looking at this again, it seems you drop the mixed up columns anyway (https://github.com/scverse/squidpy/blob/fb069ded7515e8a1386224d32344a8657cbecd2e/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

[giovp]

@giovp looking at this again, it seems you drop the mixed up columns anyway (scverse/squidpy@fb069de/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

Hi @stephenwilliams22 ,

we drop it from obs since we don't want to save spatial coordinates there, cause they should be only in obsm.

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

[ivirshup]

@giovp, should this change happen in scanpy or squidpy?

[giovp]

@giovp, should this change happen in scanpy or squidpy?

I would make it happen in both and deprecate this function from the next release (as well as all the other spatial).

[ivirshup]

Has this happened in squidpy already?

[giovp]

Has this happened in squidpy already?

yes, need one further clarification from @stephenwilliams22 and then will resolve also in squidpy.

[stephenwilliams22]

@giovp looking at this again, it seems you drop the mixed up columns anyway (scverse/squidpy@fb069de/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

Hi @stephenwilliams22 ,

we drop it from obs since we don't want to save spatial coordinates there, cause they should be only in obsm.

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

Yes, this is correct.

[stephenwilliams22]

@giovp do we have an update on merging this yet? Anything left on my end?

[ivirshup]

@giovp, @stephenwilliams22, @Zethson

This PR seems to have broken a number of tests. I believe due to the change in pixel_row/ pixel_col order, resulting in a number of test plots now being transposed.

If this was always wrong... surely usage would have caught that, so we must be correcting for this somewhere else.

What's the correction here? And is it something that should be done quickly, or should I revert this PR until we can figure it out?

[giovp]

from discussing with @stephenwilliams22 this made me think there shouldn't be changes with column names yet it seems like it is,

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

Yes, this is correct.

we actually might have had similar reports in squidpy as well, let me ask @LLehren for support, will probably have to fix it in squidpy side as well.

[giovp]

for reference scverse/squidpy#599

[ivirshup]

Alright, I'm going to revert for now then

[stephenwilliams22]

This PR updates scanpy to accommodate the latest changes made in spaceranger 2.0 which will break the released version of scanpy. Will provide backwards compatibility to pre 2.0 releases.

[codecov]

Codecov Report

Merging #2296 (51a9126) into master (0fec570) will increase coverage by 71.49%.
The diff coverage is 100.00%.

❗ Current head 51a9126 differs from pull request most recent head 241ebb5. Consider uploading reports for the commit 241ebb5 to get more accurate results

Additional details and impacted files

@@ Coverage Diff @@## master #2296 +/- ##
===========================================
+ Coverage 0.31% 71.80% +71.49% 
===========================================
Files 97 97 Lines 11485 11523 +38 ===========================================
+ Hits 36 8274 +8238 + Misses 11449 3249 -8200 

Impacted Files	Coverage Δ
scanpy/readwrite.py	67.74% <100.00%> (+67.74%)	⬆️
scanpy/__init__.py
scanpy/_utils/compute/is_constant.py	70.76% <0.00%> (ø)
scanpy/external/exporting.py	11.86% <0.00%> (+11.86%)	⬆️
scanpy/tools/_louvain.py	20.21% <0.00%> (+20.21%)	⬆️
scanpy/external/tl/_wishbone.py	21.05% <0.00%> (+21.05%)	⬆️
scanpy/external/tl/_palantir.py	22.58% <0.00%> (+22.58%)	⬆️
scanpy/external/tl/_harmony_timeseries.py	25.00% <0.00%> (+25.00%)	⬆️
scanpy/external/tl/_trimap.py	25.92% <0.00%> (+25.92%)	⬆️
scanpy/external/tl/_pypairs.py	26.47% <0.00%> (+26.47%)	⬆️
... and 86 more

[Zethson]

Thank you! Maybe also relevant for @giovp ?

[stephenwilliams22]

@Zethson just wanted to see if there were any issues with merging this PR.

[giovp]

hi @stephenwilliams22 , thanks for this PR, only one minor thing.

[stephenwilliams22]

@giovp looking at this again, it seems you drop the mixed up columns anyway (https://github.com/scverse/squidpy/blob/fb069ded7515e8a1386224d32344a8657cbecd2e/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

[giovp]

@giovp looking at this again, it seems you drop the mixed up columns anyway (scverse/squidpy@fb069de/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

Hi @stephenwilliams22 ,

we drop it from obs since we don't want to save spatial coordinates there, cause they should be only in obsm.

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

[ivirshup]

@giovp, should this change happen in scanpy or squidpy?

[giovp]

@giovp, should this change happen in scanpy or squidpy?

I would make it happen in both and deprecate this function from the next release (as well as all the other spatial).

[ivirshup]

Has this happened in squidpy already?

[giovp]

Has this happened in squidpy already?

yes, need one further clarification from @stephenwilliams22 and then will resolve also in squidpy.

[stephenwilliams22]

@giovp looking at this again, it seems you drop the mixed up columns anyway (scverse/squidpy@fb069de/squidpy/read/_read.py#L100) so I'd suggest having one code path with the correct column names. Please let me know if you need more clarity and help getting this over the hump for release.

Hi @stephenwilliams22 ,

we drop it from obs since we don't want to save spatial coordinates there, cause they should be only in obsm.

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

Yes, this is correct.

[stephenwilliams22]

@giovp do we have an update on merging this yet? Anything left on my end?

[ivirshup]

@giovp, @stephenwilliams22, @Zethson

This PR seems to have broken a number of tests. I believe due to the change in pixel_row/ pixel_col order, resulting in a number of test plots now being transposed.

If this was always wrong... surely usage would have caught that, so we must be correcting for this somewhere else.

What's the correction here? And is it something that should be done quickly, or should I revert this PR until we can figure it out?

[giovp]

from discussing with @stephenwilliams22 this made me think there shouldn't be changes with column names yet it seems like it is,

So the only changes between space rangers version is the name of the tissue position file correct? but the order of the columns remains the same?

Yes, this is correct.

we actually might have had similar reports in squidpy as well, let me ask @LLehren for support, will probably have to fix it in squidpy side as well.

[giovp]

for reference scverse/squidpy#599

[ivirshup]

Alright, I'm going to revert for now then