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

added quantisation factor and RCC4 #195

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pet1330
Copy link
Member

@pet1330 pet1330 commented Sep 28, 2015

Added quantisation factor to demonstrate RCC4 relations to {RCC}2,3,8

@pet1330 pet1330 mentioned this pull request Sep 28, 2015
@gatsoulis
Copy link
Contributor

Thanks, I think it will be nice. But, what is it for? :) I thought q-factor applies to RCC8, then the simplified RCCs just do a simple mapping. Correct?

Also, let me emphasize that I've seen the previous debugging interface and it is pretty cool, but in order to test rcc4 or any mapping from rcc8 QSR, isn't it as straightforward as looking in their mapping function/dictionary? For example for current RCC4 PR, see here and here.

If there anything wrong other than the conceptual mapping, then the bug would lie with RCC8. Am I missing something?

In any case do you want me to accept this PR if it passes the tests? Note that the jenkins tests are not complete at this stage, (removed the multiple-test for the time being till the other two QSRs are merged), but even without the multiple if it passes the rest I don't see any danger to the core.

@gatsoulis
Copy link
Contributor

Happy to merge. Unless you want to add rcc5 string also first in the two appropriate lines?

@pet1330
Copy link
Member Author

pet1330 commented Sep 28, 2015

Yeah it does, all the relations logic is done in the abstract class, which is basically the RCC8 relations, then the rest of the RCC types just do a lookup for the relevant relations. However, if you change the conversions, it's important to test both with and without quantisation, as sometimes this can lead to misleading relations.

Comment from #187:

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.

I'll add and test RCC5 and 3 tomorrow morning so we can get these both of these merged by tomorrow afternoon. 😄

@pet1330
Copy link
Member Author

pet1330 commented Sep 28, 2015

As a side note, I put this in scripts as it's a bit more general purpose then just debug, but I wasn't sure where it belongs. I will at some point separate it out into different files and create an interface for it so it can be called directly, maybe as an argument for the library? I also need to add QTC which currently isn't supported as it requires two timestamps... 👎 But these are all on my todo list when I get more time.

@gatsoulis
Copy link
Contributor

IMO there should be a qsr_visualizations python package lying the src folder. This means that

if __name__ == "__main__":
     vis = qsr_gui()
     vis.processArgs()
     vis.initWindow()
     vis.initWindow() 

should be in a script in the scripts folder (you can still leave it in the class file).

@gatsoulis
Copy link
Contributor

I'll add and test RCC5 and 3 tomorrow morning so we can get these both of these merged by tomorrow afternoon.

Thanks.

@pet1330
Copy link
Member Author

pet1330 commented Sep 29, 2015

Sorry, got waylaid today, I'll get this done tomorrow! 😄

@pet1330
Copy link
Member Author

pet1330 commented Sep 30, 2015

I've got a few errors with this one, that need to be fixed. Don't. merge it yet...

@hawesie
Copy link
Member

hawesie commented Jul 30, 2020

Did those errors get fixed @pet1330 ?

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.

3 participants