Skip to content

DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction - #369

Open
mrawls wants to merge 1 commit into
mainfrom
tickets/DM-48256
Open

DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction#369
mrawls wants to merge 1 commit into
mainfrom
tickets/DM-48256

Conversation

@mrawls

@mrawlsmrawls commented Dec 24, 2024

Copy link
Copy Markdown
Contributor

No description provided.

@mrawls
mrawls requested a review from parejkojDecember 24, 2024 07:56
parejkoj
parejkoj requested changes Jan 7, 2025
View reviewed changes

@parejkojparejkoj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we get a unittest that confirms the bug is fixed? Because as coded, this wouldn't fix it (and I'm now not sure what the underlying bug truly is anyway!).

Comment threadpython/lsst/ip/diffim/subtractImages.py Outdated
xv = (np.rint(sources.getX() - mask.getX0())).astype(int)
yv = (np.rint(sources.getY() - mask.getY0())).astype(int)

# The source catalog may not be contiguous.

@parejkojparejkojJan 7, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think this comment is necessary: the lines below say the same thing.

Comment threadpython/lsst/ip/diffim/subtractImages.py Outdated
yv = (np.rint(sources.getY() - mask.getY0())).astype(int)

# The source catalog may not be contiguous.
if not sources.isContiguous():

@parejkojparejkojJan 7, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure this is too late to fix the problem, since the error report has it coming from the xv=... line above. When I said line 854, it must have been on a different version of the file. What I had meant was just after the call to self.sourceSelector.selectSources in _sourceSelector(). Except _checkMask is called on sources, not selected, so I don't see how it could fail in this way, unless the input catalog is broken somehow?

@mrawlsmrawlsJan 9, 2025

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 put the putative fix in _checkMask because it operates on the source table in question, but you're probably right that this would best be handled earlier.

@mrawls

mrawls commented Jan 9, 2025

Copy link
Copy Markdown
ContributorAuthor

I'd like to add a unit test but I'm not sure how to make a non-contiguous source catalog on purpose. I think the test would best go here; I can work on this tomorrow.

@parejkoj

parejkoj commented Jan 10, 2025

Copy link
Copy Markdown
Contributor

I think we need to find out how this was triggered in this first place. I posted a followup question on the ticket, because it doesn't seem possible to have failed with a non-contiguous, unmodified catalog.

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

Reviewers

@parejkojparejkojparejkoj requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mrawls@parejkoj
, '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" + '
DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction by mrawls · Pull Request #369 · lsst/ip_diffim · GitHub
Skip to content
DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction - #369
Open
mrawls wants to merge 1 commit into
mainfrom 
tickets/DM-48256
Open
DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction#369
mrawls wants to merge 1 commit into
mainfrom 
tickets/DM-48256
Conversation
@mrawls
@mrawlsmrawls
commented
Dec 24, 2024

Copy link


Copy Markdown

Contributor
 No description provided. 
@mrawls
mrawls
requested a review
from parejkojDecember 24, 2024 07:56
parejkoj
parejkoj
requested changes
Jan 7, 2025
View reviewed changes
@parejkojparejkoj
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could we get a unittest that confirms the bug is fixed? Because as coded, this wouldn't fix it (and I'm now not sure what the underlying bug truly is anyway!).
Comment threadpython/lsst/ip/diffim/subtractImages.py
Outdated

 xv = (np.rint(sources.getX() - mask.getX0())).astype(int)
 yv = (np.rint(sources.getY() - mask.getY0())).astype(int)

 # The source catalog may not be contiguous.
@parejkojparejkojJan 7, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I don't think this comment is necessary: the lines below say the same thing.
Comment threadpython/lsst/ip/diffim/subtractImages.py
Outdated

 yv = (np.rint(sources.getY() - mask.getY0())).astype(int)

 # The source catalog may not be contiguous.
 if not sources.isContiguous():
@parejkojparejkojJan 7, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm pretty sure this is too late to fix the problem, since the error report has it coming from the xv=... line above. When I said line 854, it must have been on a different version of the file. What I had meant was just after the call to self.sourceSelector.selectSources in _sourceSelector(). Except _checkMask is called on sources, not selected, so I don't see how it could fail in this way, unless the input catalog is broken somehow?
@mrawlsmrawlsJan 9, 2025

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 put the putative fix in _checkMask because it operates on the source table in question, but you're probably right that this would best be handled earlier.
@mrawls
mrawls
commented
Jan 9, 2025

Copy link


Copy Markdown

ContributorAuthor
I'd like to add a unit test but I'm not sure how to make a non-contiguous source catalog on purpose. I think the test would best go here; I can work on this tomorrow.
@parejkoj
parejkoj
commented
Jan 10, 2025

Copy link


Copy Markdown

Contributor
I think we need to find out how this was triggered in this first place. I posted a followup question on the ticket, because it doesn't seem possible to have failed with a non-contiguous, unmodified catalog.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@parejkojparejkojparejkoj requested changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@mrawls@parejkoj

, '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('^' + ".*" + '
DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction by mrawls · Pull Request #369 · lsst/ip_diffim · GitHub
Skip to content
DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction - #369
Open
mrawls wants to merge 1 commit into
mainfrom 
tickets/DM-48256
Open
DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction#369
mrawls wants to merge 1 commit into
mainfrom 
tickets/DM-48256
Conversation
@mrawls
@mrawlsmrawls
commented
Dec 24, 2024

Copy link


Copy Markdown

Contributor
 No description provided. 
@mrawls
mrawls
requested a review
from parejkojDecember 24, 2024 07:56
parejkoj
parejkoj
requested changes
Jan 7, 2025
View reviewed changes
@parejkojparejkoj
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could we get a unittest that confirms the bug is fixed? Because as coded, this wouldn't fix it (and I'm now not sure what the underlying bug truly is anyway!).
Comment threadpython/lsst/ip/diffim/subtractImages.py
Outdated

 xv = (np.rint(sources.getX() - mask.getX0())).astype(int)
 yv = (np.rint(sources.getY() - mask.getY0())).astype(int)

 # The source catalog may not be contiguous.
@parejkojparejkojJan 7, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I don't think this comment is necessary: the lines below say the same thing.
Comment threadpython/lsst/ip/diffim/subtractImages.py
Outdated

 yv = (np.rint(sources.getY() - mask.getY0())).astype(int)

 # The source catalog may not be contiguous.
 if not sources.isContiguous():
@parejkojparejkojJan 7, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm pretty sure this is too late to fix the problem, since the error report has it coming from the xv=... line above. When I said line 854, it must have been on a different version of the file. What I had meant was just after the call to self.sourceSelector.selectSources in _sourceSelector(). Except _checkMask is called on sources, not selected, so I don't see how it could fail in this way, unless the input catalog is broken somehow?
@mrawlsmrawlsJan 9, 2025

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 put the putative fix in _checkMask because it operates on the source table in question, but you're probably right that this would best be handled earlier.
@mrawls
mrawls
commented
Jan 9, 2025

Copy link


Copy Markdown

ContributorAuthor
I'd like to add a unit test but I'm not sure how to make a non-contiguous source catalog on purpose. I think the test would best go here; I can work on this tomorrow.
@parejkoj
parejkoj
commented
Jan 10, 2025

Copy link


Copy Markdown

Contributor
I think we need to find out how this was triggered in this first place. I posted a followup question on the ticket, because it doesn't seem possible to have failed with a non-contiguous, unmodified catalog.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@parejkojparejkojparejkoj requested changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@mrawls@parejkoj

, '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('^' + ".*" + '
DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction by mrawls · Pull Request #369 · lsst/ip_diffim · GitHub
Skip to content
DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction - #369
Open
mrawls wants to merge 1 commit into
mainfrom 
tickets/DM-48256
Open
DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction#369
mrawls wants to merge 1 commit into
mainfrom 
tickets/DM-48256
Conversation
@mrawls
@mrawlsmrawls
commented
Dec 24, 2024

Copy link


Copy Markdown

Contributor
 No description provided. 
@mrawls
mrawls
requested a review
from parejkojDecember 24, 2024 07:56
parejkoj
parejkoj
requested changes
Jan 7, 2025
View reviewed changes
@parejkojparejkoj
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could we get a unittest that confirms the bug is fixed? Because as coded, this wouldn't fix it (and I'm now not sure what the underlying bug truly is anyway!).
Comment threadpython/lsst/ip/diffim/subtractImages.py
Outdated

 xv = (np.rint(sources.getX() - mask.getX0())).astype(int)
 yv = (np.rint(sources.getY() - mask.getY0())).astype(int)

 # The source catalog may not be contiguous.
@parejkojparejkojJan 7, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I don't think this comment is necessary: the lines below say the same thing.
Comment threadpython/lsst/ip/diffim/subtractImages.py
Outdated

 yv = (np.rint(sources.getY() - mask.getY0())).astype(int)

 # The source catalog may not be contiguous.
 if not sources.isContiguous():
@parejkojparejkojJan 7, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm pretty sure this is too late to fix the problem, since the error report has it coming from the xv=... line above. When I said line 854, it must have been on a different version of the file. What I had meant was just after the call to self.sourceSelector.selectSources in _sourceSelector(). Except _checkMask is called on sources, not selected, so I don't see how it could fail in this way, unless the input catalog is broken somehow?
@mrawlsmrawlsJan 9, 2025

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 put the putative fix in _checkMask because it operates on the source table in question, but you're probably right that this would best be handled earlier.
@mrawls
mrawls
commented
Jan 9, 2025

Copy link


Copy Markdown

ContributorAuthor
I'd like to add a unit test but I'm not sure how to make a non-contiguous source catalog on purpose. I think the test would best go here; I can work on this tomorrow.
@parejkoj
parejkoj
commented
Jan 10, 2025

Copy link


Copy Markdown

Contributor
I think we need to find out how this was triggered in this first place. I posted a followup question on the ticket, because it doesn't seem possible to have failed with a non-contiguous, unmodified catalog.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@parejkojparejkojparejkoj requested changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@mrawls@parejkoj

, '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" + '
DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction by mrawls · Pull Request #369 · lsst/ip_diffim · GitHub
Skip to content
DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction - #369
Open
mrawls wants to merge 1 commit into
mainfrom 
tickets/DM-48256
Open
DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction#369
mrawls wants to merge 1 commit into
mainfrom 
tickets/DM-48256
Conversation
@mrawls
@mrawlsmrawls
commented
Dec 24, 2024

Copy link


Copy Markdown

Contributor
 No description provided. 
@mrawls
mrawls
requested a review
from parejkojDecember 24, 2024 07:56
parejkoj
parejkoj
requested changes
Jan 7, 2025
View reviewed changes
@parejkojparejkoj
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could we get a unittest that confirms the bug is fixed? Because as coded, this wouldn't fix it (and I'm now not sure what the underlying bug truly is anyway!).
Comment threadpython/lsst/ip/diffim/subtractImages.py
Outdated

 xv = (np.rint(sources.getX() - mask.getX0())).astype(int)
 yv = (np.rint(sources.getY() - mask.getY0())).astype(int)

 # The source catalog may not be contiguous.
@parejkojparejkojJan 7, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I don't think this comment is necessary: the lines below say the same thing.
Comment threadpython/lsst/ip/diffim/subtractImages.py
Outdated

 yv = (np.rint(sources.getY() - mask.getY0())).astype(int)

 # The source catalog may not be contiguous.
 if not sources.isContiguous():
@parejkojparejkojJan 7, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm pretty sure this is too late to fix the problem, since the error report has it coming from the xv=... line above. When I said line 854, it must have been on a different version of the file. What I had meant was just after the call to self.sourceSelector.selectSources in _sourceSelector(). Except _checkMask is called on sources, not selected, so I don't see how it could fail in this way, unless the input catalog is broken somehow?
@mrawlsmrawlsJan 9, 2025

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 put the putative fix in _checkMask because it operates on the source table in question, but you're probably right that this would best be handled earlier.
@mrawls
mrawls
commented
Jan 9, 2025

Copy link


Copy Markdown

ContributorAuthor
I'd like to add a unit test but I'm not sure how to make a non-contiguous source catalog on purpose. I think the test would best go here; I can work on this tomorrow.
@parejkoj
parejkoj
commented
Jan 10, 2025

Copy link


Copy Markdown

Contributor
I think we need to find out how this was triggered in this first place. I posted a followup question on the ticket, because it doesn't seem possible to have failed with a non-contiguous, unmodified catalog.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@parejkojparejkojparejkoj requested changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@mrawls@parejkoj

, '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('^' + ".*" + '
DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction by mrawls · Pull Request #369 · lsst/ip_diffim · GitHub
Skip to content
DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction - #369
Open
mrawls wants to merge 1 commit into
mainfrom 
tickets/DM-48256
Open
DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction#369
mrawls wants to merge 1 commit into
mainfrom 
tickets/DM-48256
Conversation
@mrawls
@mrawlsmrawls
commented
Dec 24, 2024

Copy link


Copy Markdown

Contributor
 No description provided. 
@mrawls
mrawls
requested a review
from parejkojDecember 24, 2024 07:56
parejkoj
parejkoj
requested changes
Jan 7, 2025
View reviewed changes
@parejkojparejkoj
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could we get a unittest that confirms the bug is fixed? Because as coded, this wouldn't fix it (and I'm now not sure what the underlying bug truly is anyway!).
Comment threadpython/lsst/ip/diffim/subtractImages.py
Outdated

 xv = (np.rint(sources.getX() - mask.getX0())).astype(int)
 yv = (np.rint(sources.getY() - mask.getY0())).astype(int)

 # The source catalog may not be contiguous.
@parejkojparejkojJan 7, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I don't think this comment is necessary: the lines below say the same thing.
Comment threadpython/lsst/ip/diffim/subtractImages.py
Outdated

 yv = (np.rint(sources.getY() - mask.getY0())).astype(int)

 # The source catalog may not be contiguous.
 if not sources.isContiguous():
@parejkojparejkojJan 7, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm pretty sure this is too late to fix the problem, since the error report has it coming from the xv=... line above. When I said line 854, it must have been on a different version of the file. What I had meant was just after the call to self.sourceSelector.selectSources in _sourceSelector(). Except _checkMask is called on sources, not selected, so I don't see how it could fail in this way, unless the input catalog is broken somehow?
@mrawlsmrawlsJan 9, 2025

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 put the putative fix in _checkMask because it operates on the source table in question, but you're probably right that this would best be handled earlier.
@mrawls
mrawls
commented
Jan 9, 2025

Copy link


Copy Markdown

ContributorAuthor
I'd like to add a unit test but I'm not sure how to make a non-contiguous source catalog on purpose. I think the test would best go here; I can work on this tomorrow.
@parejkoj
parejkoj
commented
Jan 10, 2025

Copy link


Copy Markdown

Contributor
I think we need to find out how this was triggered in this first place. I posted a followup question on the ticket, because it doesn't seem possible to have failed with a non-contiguous, unmodified catalog.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@parejkojparejkojparejkoj requested changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@mrawls@parejkoj

, '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); }
})();
})();
DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction by mrawls · Pull Request #369 · lsst/ip_diffim · GitHub
Skip to content
DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction - #369
Open
mrawls wants to merge 1 commit into
mainfrom 
tickets/DM-48256
Open
DM-48256: Fix RunTimeError 'Record data is not contiguous in memory.' when running subtraction#369
mrawls wants to merge 1 commit into
mainfrom 
tickets/DM-48256
Conversation
@mrawls
@mrawlsmrawls
commented
Dec 24, 2024

Copy link


Copy Markdown

Contributor
 No description provided. 
@mrawls
mrawls
requested a review
from parejkojDecember 24, 2024 07:56
parejkoj
parejkoj
requested changes
Jan 7, 2025
View reviewed changes
@parejkojparejkoj
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could we get a unittest that confirms the bug is fixed? Because as coded, this wouldn't fix it (and I'm now not sure what the underlying bug truly is anyway!).
Comment threadpython/lsst/ip/diffim/subtractImages.py
Outdated

 xv = (np.rint(sources.getX() - mask.getX0())).astype(int)
 yv = (np.rint(sources.getY() - mask.getY0())).astype(int)

 # The source catalog may not be contiguous.
@parejkojparejkojJan 7, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I don't think this comment is necessary: the lines below say the same thing.
Comment threadpython/lsst/ip/diffim/subtractImages.py
Outdated

 yv = (np.rint(sources.getY() - mask.getY0())).astype(int)

 # The source catalog may not be contiguous.
 if not sources.isContiguous():
@parejkojparejkojJan 7, 2025

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm pretty sure this is too late to fix the problem, since the error report has it coming from the xv=... line above. When I said line 854, it must have been on a different version of the file. What I had meant was just after the call to self.sourceSelector.selectSources in _sourceSelector(). Except _checkMask is called on sources, not selected, so I don't see how it could fail in this way, unless the input catalog is broken somehow?
@mrawlsmrawlsJan 9, 2025

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 put the putative fix in _checkMask because it operates on the source table in question, but you're probably right that this would best be handled earlier.
@mrawls
mrawls
commented
Jan 9, 2025

Copy link


Copy Markdown

ContributorAuthor
I'd like to add a unit test but I'm not sure how to make a non-contiguous source catalog on purpose. I think the test would best go here; I can work on this tomorrow.
@parejkoj
parejkoj
commented
Jan 10, 2025

Copy link


Copy Markdown

Contributor
I think we need to find out how this was triggered in this first place. I posted a followup question on the ticket, because it doesn't seem possible to have failed with a non-contiguous, unmodified catalog.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@parejkojparejkojparejkoj requested changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@mrawls@parejkoj