-
Notifications
You must be signed in to change notification settings - Fork 2
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
Algorithm improvements #6
base: master
Are you sure you want to change the base?
Conversation
Implemented math.js library. Implemented sparse matrices for storing distances and modified code to only calculate distances that are non-existent.
* master: Fixed random point generator. Fix formatting of probabilities Pull point creation into separate function. Take square root of random number times radius squared to ensure point stays inside circle, fixing issue #4 Fix issues #2, #5 Fix issue #1 # Conflicts: # .idea/Working version/script.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating the potential node locations doesn't update the probabilities. Similarly, when you move a node the probabilities and edges don't update.
|
||
} | ||
|
||
function intersection(a, b, c) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this method used? I don't see anywhere it is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was using this function for other improvements I didn't finish. It's not needed at the moment but will ultimately be helpful.
if ( p > 0 ) { | ||
// if edge already exists just update probability | ||
if ( edgeInd && radiusChange==1 ) { | ||
tempEdges[edgeInd-1].Pedge = p; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I increase the coverage radius, sometimes I get an error saying "Uncaught TypeError: Cannot set property 'Pedge' of undefined." It seems that edgeInd is a higher number than what is in the tempEdges.
var sqDiameterMax = 4 * Math.pow(complexRadius+dataRadius, 2); | ||
|
||
|
||
if (!a_edges) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because a_edges is only populated on file load, clearing the screen completely breaks it. For example, I have attached a picture of what happened when I cleared the screen and added nodes. It never generated edges because a_edges was just zeros. Faces appeared, but no edges. Only after increasing the coverage radius to be huge did it find one edge when there should have been way more.
var iEdgeDist, iEdgeFlag, tempEdges; | ||
|
||
if ( radiusChange==1 ) { | ||
tempEdges = JSON.parse(JSON.stringify(ripsEdges)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to do this? When you load in the file with d3.json it already formats it into an array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was actually made to the main branch before I started working on the algorithm and done to make a copy of the array. Originally I was using array.slice(), but while it makes a shallow copy of the array, with object arrays it just copies the object references, so any changes affect the original array. This is what was causing that issue earlier with the Rips probabilities getting overwritten by the Cech probabilities. After some research, this seems to be the cleanest way of making a deep copy and leaving the original array intact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could possibly use Object.assign (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign). The only downside with this is that it doesn't support Internet Explorer. I'm not sure if we wanted to make sure it worked on everything or add that stipulation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like object assign works only on the objects themselves, not arrays. I would still have to loop over the array using map or forEach. The approach I used seems to be the most recommended way to do this. https://stackoverflow.com/questions/41273735/how-do-i-copy-an-array-of-arrays-of-objects-and-be-able-to-manipulate-the-copied
As for compatibility, I know we never looked at it, but ultimately I would say we want cross-browser compatibility so probably best not to implement new functions that we know won't work on IE. I will worry about that part.
constructRips(); | ||
|
||
|
||
if (radiusChange == 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could change this to just be "newValue > complexRadius" and remove the entire variable radiusChange
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, could be cleaned up a bit but not super important right now. I do ultimately plan on passing the radius change in as an argument so the algorithm knows which points to look at but this will be in a later version.
|
||
} | ||
|
||
function updateRips() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the point of this function? Is it ever used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code snippet I abandoned. I can get rid of it.
function constructCech() { | ||
|
||
|
||
cechEdges = JSON.parse(JSON.stringify(ripsEdges)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to stringify/parse because ripEdges is already parsed when you loaded the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comment.
cechEdges = JSON.parse(JSON.stringify(ripsEdges)); | ||
cechFaces = []; | ||
var newFaces = []; | ||
var tempFaces = JSON.parse(JSON.stringify(ripsFaces)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to stringify/parse because ripFaces is already parsed when you loaded the file.
No description provided.