Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

try quick fix on #1136 #1137

Closed
wants to merge 3 commits into from
Closed

try quick fix on #1136 #1137

wants to merge 3 commits into from

Conversation

dehann
Copy link
Member

@dehann dehann commented Jan 27, 2021

Might need backports

@dehann dehann added this to the v0.21.0 milestone Jan 27, 2021
@dehann dehann added the bug fix label Jan 27, 2021
@dehann dehann linked an issue Jan 27, 2021 that may be closed by this pull request
@dehann dehann requested a review from Affie January 27, 2021 23:50
@codecov-io
Copy link

codecov-io commented Jan 28, 2021

Codecov Report

Merging #1137 (35a3d0c) into master (19afa75) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1137   +/-   ##
=======================================
  Coverage   73.20%   73.20%           
=======================================
  Files          55       55           
  Lines        4673     4673           
=======================================
  Hits         3421     3421           
  Misses       1252     1252           
Impacted Files Coverage Δ
src/CliqStateMachineUtils.jl 81.25% <100.00%> (ø)
src/GraphProductOperations.jl 94.11% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19afa75...35a3d0c. Read the comment docs.

@Affie
Copy link
Member

Affie commented Jan 28, 2021

Looks good, it needs something similar for multiprocessor.
What bothers me a bit is that fixing this will hide another potential error.

@dehann
Copy link
Member Author

dehann commented Jan 31, 2021

needs something similar for multiprocessor

I'd have to look for it, or please add if you have easy recall on where those lines of code are.

another potential error

oops, which error would that be?

@Affie
Copy link
Member

Affie commented Feb 1, 2021

oops, which error would that be?

See #1136 (comment)
There were no factors on the specific frontal.

@Affie
Copy link
Member

Affie commented Feb 1, 2021

I'd have to look for it, or please add if you have easy recall on where those lines of code are.

I added as I understand, please review.

Copy link
Member

@Affie Affie left a comment

Choose a reason for hiding this comment

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

I added same on multiproc

@Affie
Copy link
Member

Affie commented Feb 6, 2021

ping @dehann

@dehann dehann modified the milestones: v0.21.0, v0.22.0 Feb 6, 2021
@dehann dehann modified the milestones: v0.22.0, v0.21.2 Feb 18, 2021
@dehann
Copy link
Member Author

dehann commented Feb 18, 2021

hi @Affie , we going back and forth. I think we can merge although I am not sure about the concerns you raised. It looks like you cleared them with the later commit. If you are happy, please go ahead and merge this PR.

@dehann dehann modified the milestones: v0.21.2, v0.21.3 Feb 21, 2021
@Affie
Copy link
Member

Affie commented Feb 23, 2021

The concerns are about an underlying issue that causes the point to be zero. (that should not happen in this case)
There was a warning a while back that I think is related. I don't know if it was taken out or redirected to the logger, I'll try and find it. The problem is to find MWE that shows this. Give me some time on this one as I don't want to potentially hide an issue.

@dehann dehann modified the milestones: v0.21.3, v0.21.x Feb 24, 2021
@dehann
Copy link
Member Author

dehann commented Feb 24, 2021

okay, understood thanks!

@Affie
Copy link
Member

Affie commented Feb 25, 2021

Ok, I kind of remember (70%) the waning being from here:

@warn "Unknown density product on variable=$(vert.label), lennonp=$(lennonp), lenpart=$(lenpart)"

@dehann dehann modified the milestones: v0.21.x, v0.22.x Mar 27, 2021
@dehann dehann marked this pull request as draft April 10, 2021 15:53
@dehann dehann modified the milestones: v0.24.x, v0.0.x Jun 18, 2021
@dehann dehann modified the milestones: v0.0.x, v0.27.0 Jan 31, 2022
@dehann
Copy link
Member Author

dehann commented Jan 31, 2022

error #1136 was caused by keyword arguments in AMP.manifoldProduct not having enough content for broadcasting. A few alternate PRs should have fixed the issue. The attempted fix in this PR is not valid, and would introduce an unwanted error.

@dehann dehann closed this Jan 31, 2022
@dehann dehann deleted the 21Q1/hotfix/1136 branch November 27, 2024 14:02
Sign up for free to 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.

Attempt to access 0-element Vector{Float64} at index [1]
3 participants