uscuni / sgeop

Street geometry processing toolkit
BSD 3-Clause "New" or "Revised" License
13 stars 0 forks source link

[Question] node + continuity group explanations from `artifacts.py` #99

Closed jGaboardi closed 1 day ago

jGaboardi commented 5 days ago

@martinfleis & @anastassiavybornova Can you check/confirm my understanding of these?

Drop/add lines within artifacts when...

jGaboardi commented 5 days ago

@martinfleis Is my understanding here correct?

martinfleis commented 5 days ago
anastassiavybornova commented 5 days ago

@martinfleis @jGaboardi

my first intuition is also that the "identical" refers to identical continuity groups (ike, 4CCC, or 5EE, or 3SSS), and then nx_gx deals with "mixed" continuity groups.

my second intutition is that "identical" refers to: number of nodes == number of continuity groups (ie there are no additional "to be preserved" nodes that we have to deal with).

the fact that we are using "identical" in "n1_g1_identical" is in my view a hint towards the 2nd intuition.

@martinfleis what does your saturday brain say?

i now have a slight discomfort of how we're using "x" in the function name cause it stands at first use for number of nodes (nx) and then at second use for number of continuity grops (gx) however they are not necessarily equal (but can be). so maybe instead of the above function names with "identical" and "x" we can use:

martinfleis commented 4 days ago

my first intuition is also that the "identical" refers to identical continuity groups (ike, 4CCC, or 5EE, or 3SSS), and then nx_gx deals with "mixed" continuity groups.

My Saturday brain is convinced that this is the case.

jGaboardi commented 4 days ago

So "first intuition" is the winner?

jGaboardi commented 4 days ago

so maybe instead of the above function names with "identical" and "x" we can use:

  • n1_g1
  • nx_gx (x==x)
  • nx_gy (x!=y)

@anastassiavybornova This is clean. I like it.

@martinfleis Do you have any hesitations for renaming like so?

martinfleis commented 4 days ago

Yeah, the first is correct. I don't have how we name it.

jGaboardi commented 4 days ago

I'll move forward with the following translation:

martinfleis commented 4 days ago

That switches the nx_gx name to a different function. Dangerous, can we not?

anastassiavybornova commented 4 days ago

@jGaboardi if the first (but NOT the second) intuition is correct, then renaming nx_gx_identical to nx_gx (x==x) is not correct (cause identical refers to the continuity groups, and not to the number of continuity groups vs nodes). so in addition to @martinfleis understandable concerns above it also introduces my false 2nd intuition here. therefore:

how about instead:

option b would be to learn to live with the fact that x can have identical or differing values :D and keep the naming as is now (documenting what exactly we mean)

jGaboardi commented 3 days ago

That switches the nx_gx name to a different function. Dangerous, can we not?

Good point. I misunderstood. 😵‍💫

jGaboardi commented 3 days ago

renaming nx_gx_identical to nx_gx (x==x) is not correct (cause identical refers to the continuity groups, and not to the number of continuity groups vs nodes)

Got it. I think I was looking at this too long yesterday.

I like these!

  • n1_g1_identical --> n1_g1
  • nx_gx_identical --> nx_gy_samecontinuity or smth similar
  • nx_gx --> nx_gy
  • nx_gx_cluster --> nx_gy_cluster

How about nx_gx_identical --> nx_gy_eq_cgroup? Is that too long? "equivalent continuity group"

martinfleis commented 3 days ago

nx_gx_identical is not for the same continuity group. It is used when there are at least two nodes, one or more continuity groups but all edges have the same position in their respective continuity groups. i.e they all are E (ending), or S (single). It does not mean that all edges belong to a single continuity group.

I don't think the current naming is problematic and would probably prefer not to touch it. We can be explicit in docstring. And keep in mind that all this is private.

jGaboardi commented 3 days ago

nx_gx_identical is not for the same continuity group. It is used when there are at least two nodes, one or more continuity groups but all edges have the same position in their respective continuity groups. i.e they all are E (ending), or S (single). It does not mean that all edges belong to a single continuity group.

I don't think the current naming is problematic and would probably prefer not to touch it. We can be explicit in docstring. And keep in mind that all this is private.

Reasonable