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

DeleteVariable should clean up _variableOrderSymbols in neighbor factors #20

Closed
GearsAD opened this issue Jun 1, 2019 · 7 comments
Closed

Comments

@GearsAD
Copy link
Collaborator

GearsAD commented Jun 1, 2019

No description provided.

@GearsAD GearsAD self-assigned this Jun 1, 2019
@GearsAD GearsAD added the crud label Jun 1, 2019
@dehann dehann added this to the v0.1.x milestone Jun 10, 2019
@dehann dehann modified the milestones: v0.1.x, v0.2.x Jun 23, 2019
@GearsAD GearsAD modified the milestones: v0.2.x, v0.4.0 Sep 3, 2019
@dehann dehann modified the milestones: v0.4.0, v0.4.1 Oct 10, 2019
@dehann dehann modified the milestones: v0.4.1, v0.5.x Nov 18, 2019
@Affie
Copy link
Member

Affie commented Feb 7, 2020

I hope I understand _variableOrderSymbols correctly.
From how I see it, it is attached to a factor's definition and should not be modified at all.
How else would you know a factor is orphaned and does not have all the variables associated with it?
Its important in IIF also and used there as:
getVariableOrder(fct::DFGFactor)::Vector{Symbol} = fct._variableOrderSymbols

@Affie Affie modified the milestones: v0.5.x, v0.6.0 Feb 7, 2020
@dehann
Copy link
Member

dehann commented Feb 12, 2020

at least one major use is that factors are order sensitive to the variables being passed in and this is where that info is stored

@Affie
Copy link
Member

Affie commented Feb 12, 2020

DeleteVariable should clean up _variableOrderSymbols in neighbor factors

I would say a definite no on this, and close issue?

@Affie Affie closed this as completed Feb 12, 2020
@Affie Affie reopened this Feb 12, 2020
@Affie
Copy link
Member

Affie commented Feb 12, 2020

Sorry didn't mean to close

@dehann
Copy link
Member

dehann commented Feb 12, 2020

umm i think it’s a definite yes — perhaps we thinking of different things. if you delete a variable, then it cannot possibly exist in a factor lookup? means the factor is incomplete and should fail if evaluated. default should probably be to also delete factors attached to the deleted variable too, no?

@dehann
Copy link
Member

dehann commented Feb 12, 2020

As usual, @Affie convinced me to NOT delete from _variableOrderSymbols, and that the error should be found by checking the current neighbors to a factor.

IF a variable is deleted from DFG object, then default is to delete surrounding factors also. This can be avoided with a keyword argument.

If a factor is not deleted and a new variable is introduced, then a new factor must be created by one of the verbs and replace the current factor which has (by this time) a stale but immutable _variableOrderList.

One reason is that deleting and adding variables will not preserve the all important order. Another reason is that deleting both the edges in the graph and variables from this list undermines the test where one compares the two lists against each other -- currently used for orphan test. Third reason is when using the multhypo interface, then the number of variables is specific to that instance of the factor (as in all cases really) and therefore _variableOrderList must be immutable.

@Affie
Copy link
Member

Affie commented Feb 13, 2020

Close in favor of #296

@Affie Affie closed this as completed Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants