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

Rcc4 #187

Merged
merged 4 commits into from
Sep 30, 2015
Merged

Rcc4 #187

merged 4 commits into from
Sep 30, 2015

Conversation

gatsoulis
Copy link
Contributor

No description provided.

@gatsoulis
Copy link
Contributor Author

@pet1330 Test this plz when you have time, so I can merge it and rebase rcc5 and solve its conflicts in one go.

@gatsoulis
Copy link
Contributor Author

PS: checks will fail, but it should be the multiple only failing.

@pet1330
Copy link
Member

pet1330 commented Sep 21, 2015

Why have you left it out of the docs?

@gatsoulis
Copy link
Contributor Author

RCC3 is actually wrong in its current implementation as it does not account for inverse relations. When I used it, it was fine for the purposes I wanted it as inverse did not caused me troubles.

RCC4 is the same as RCC3 with inverse included and unified vocabulary across the RCC relations.

My suggestion would be to migrate to RCC4 from RCC3 when you have time. In no rush to deprecate it.

@pet1330
Copy link
Member

pet1330 commented Sep 21, 2015

I don't think it's wrong, I think what you're describing is a different relation.

RCC3 should have [O, PO, DC]
RCC4 you're saying should have [O, PP, PPi, DC]

That makes it a different relation. All it will mean is that I end up using RCC4 to simply combine the two PP states into the same relation. You may as well leave RCC3 as as it is and just add RCC4 with this additional functionality.

@gatsoulis
Copy link
Contributor Author

Regarding the unconventional strings of RCC3 (my fault I know) it should be [PP, PO, DC] I think. Mapping from RCC8 to RCC3 would be:

  • DC -> DC
  • EC & PO -> PO. There is a question that can be raised here whether EC is PO or DC, but in RCC4 I have it as PO as I consider an edge overlap as a partial overlap.
  • All rest, i.e. EQ, PP, NTPP, PPI, NTPPI -> PP

Is that what you mean and happy with it?

@gatsoulis
Copy link
Contributor Author

RCC8 docs

@pet1330
Copy link
Member

pet1330 commented Sep 21, 2015

Due to the way quantisation works, it needs to remain what it would have mapped to, otherwise we will get different results for each RCC when using the quantisation factor

@gatsoulis
Copy link
Contributor Author

Not sure what you mean with respect to the q-factor, it is simply a mapping from one to the other (can be done via a dict), so q-factor is kind of irrelevant to this mapping.

The current implementation maps dc to dc, ec to po, and all the rest are as o. In the o case it does not distinguish between RCC8's [po, tpp, ntpp and the inverses].

Hmmm, I think I see what you mean now, you don't want a distinction between partially overlaps and proper part of (tangential or not), but have them as occludes or better overlaps. That is fine for me but I think that in that case EC should remain as EC as transforming it to PO (partial overlap) is confusing. This would result in the following RCC3 relations [DC, EC, O] with mapping everything except DC and EC to O.

Right? If that's the case fine by me and it makes it different from RCC4 also.

@pet1330
Copy link
Member

pet1330 commented Sep 28, 2015

Check out #195, it shows you at the bottom all of the relations. When you adjust the quantisation factor, check that they are mapped correctly 😄

@gatsoulis
Copy link
Contributor Author

See my other comment, which basically I think that debugging RCC PRs that map from RCC8 is as simple as checking the mapping dictionary. So for RCC5 you can do this.

@pet1330 HOWEVER, for this PR since I had to put back RCC3, and since you are using it in your current work, you might want to take it for a spin to make sure that things still work for you. We can talk and do about minor changes at a later stage.

@pet1330
Copy link
Member

pet1330 commented Sep 30, 2015

Tested. This is okay to merge. 👍

@gatsoulis
Copy link
Contributor Author

@yianni resolve conflicts

Conflicts:
	qsr_lib/CMakeLists.txt
	qsr_lib/src/qsrlib_qsrs/__init__.py
gatsoulis pushed a commit that referenced this pull request Sep 30, 2015
@gatsoulis gatsoulis merged commit 0f34eed into strands-project:master Sep 30, 2015
@gatsoulis gatsoulis deleted the rcc4 branch September 30, 2015 15:11
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.

None yet

2 participants