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.

Sign up for GitHub

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

Jump to bottom

Required edges cannot be deleted in knowledge box after you close the knowledge box once. #1678

Closed
yasu-sh opened this issue Aug 21, 2023 · 22 comments
Closed

Required edges cannot be deleted in knowledge box after you close the knowledge box once. #1678

yasu-sh opened this issue Aug 21, 2023 · 22 comments
Labels
bug

Comments

@yasu-sh
Copy link
Contributor

yasu-sh commented Aug 21, 2023 •
edited
Loading

@jdramsey I have noticed when I try different types of knowledges among required edges.
After making settings and closing the knowledge box, you can delete manually forbidden edges, but you cannot required edges.

image

reproduction steps:

  1. creating simulation data with simulation box by default.
  2. creating knowledge box.
  3. select edge and tiers type.
  4. creating Required edges.
  5. press Done.
  6. double-click the same knowledge box.
  7. enabling Show Required Explicity check.
  8. select onw required edge
  9. pressing delete key in keyboard.
  10. pressing Yes and closing Knowledge box by pressing Done.
  11. double-click the same knowledge box again.
  12. enabling Show Required Explicity check
  13. the edges is still there. users might not notice the deletion works.

workaround:

  1. destroying model once
  2. set required edges and other settings from beginnings

TETRAD 7.5.0
Windows 10 22H2
@jdramsey
Copy link
Collaborator

@yasu-sh Oh wow, thanks. I'll have to think about that one.

@yasu-sh
Copy link
Contributor Author

yasu-sh commented Aug 23, 2023

@jdramsey I noticed this yesterday, but if you cannot reproduce it, let me know.
I need to consider some causes also.

@jdramsey
Copy link
Collaborator

@yasu-sh Hmm... I can reproduce it, it's not just you. Not sure of the cause yet.

@jdramsey
Copy link
Collaborator

jdramsey commented Oct 2, 2023

@yasu-sh OK, let me try to fix this today. Sorry for the delay; I've been inordinately busy. I finally had to list everything I needed to do and prioritize--something I don't often do!

@jdramsey
Copy link
Collaborator

jdramsey commented Oct 2, 2023

@yasu-sh Oh wait I think already fixed this! I think I'm losing my mind! In any case, it's working. I'll publish a new version of Tetrad, 7.5.1, on October 16, and it will work in that version.

@jdramsey
Copy link
Collaborator

jdramsey commented Oct 2, 2023

I'll close this since it is fixed in my branch. Let me know if you find anything else in the Tetrad UI that needs to be fixed; I'll do these fixes before October 16.

@jdramsey jdramsey closed this as completed Oct 2, 2023
@yasu-sh
Copy link
Contributor Author

yasu-sh commented Oct 3, 2023 •
edited
Loading

@jdramsey Thanks for your reminding me. I could not find the branch you said at github repos.
Could you tell me the branch name?
Today I have tried by maven build(until compile) and checked UI resposne at some branches as below:

  • development - 0ccc7f1 - Formatted the manual. - jdramsey
  • joe-work-2023-8-4 - fbf3b6f - Trying to fix time series knowledge with FCI algs. - jdramsey
  • jdramsey-patch-4 - 561aa0c - Update README.md - Joseph Ramsey

In the same window, Deleting Required Edge -> Toggle Required Explicity twice -> Deleted Required Edge appear again.

image

@jdramsey
Copy link
Collaborator

jdramsey commented Oct 3, 2023 via email

@jdramsey
Copy link
Collaborator

jdramsey commented Oct 3, 2023 via email

@jdramsey
Copy link
Collaborator

jdramsey commented Oct 3, 2023 via email

@jdramsey
Copy link
Collaborator

jdramsey commented Oct 3, 2023 via email

@jdramsey jdramsey reopened this Oct 3, 2023
@jdramsey
Copy link
Collaborator

jdramsey commented Oct 3, 2023

Everything seems to be OK. There are some changes I made in Tetrad-FX that I was planning to make in Tetrad as well; I'll do those in the next few days in the new branch.

@jdramsey
Copy link
Collaborator

jdramsey commented Oct 3, 2023

BTW thanks for catching that,.

@yasu-sh
Copy link
Contributor Author

yasu-sh commented Oct 4, 2023 •
edited
Loading

@jdramsey Thanks for checking a lot for this.
I am afraid whether I can confirm the bug fixed or not. The movie is using joe-work-2023-10-2 latest branch.
Java 11 / Adoptium\jdk-11.0.18.10 / working in IntteliJ 2023.2.2 CE.

This is not urgent. In future you might notice something to be fixed.

image
joe-work-2023-10-2

used commit
0ccc7f1

@fabiopuddu77
Copy link

fabiopuddu77 commented Oct 4, 2023 •
edited
Loading

Hello @yasu-sh you can go to file and save knowledge and then delete the required edge from there and then reload the txt :) let me know if you fix like this.

@yasu-sh
Copy link
Contributor Author

yasu-sh commented Oct 10, 2023 •
edited
Loading

@fabiopuddu77 Thanks for your action proposal.
I checked in this weekend and confirmed that the saved knowledge txt file.

  • Before [Show Required Explicity] checkbox enabled, the deleted edges in exported file is correct.
  • After enabling [Show Required Explicity], the deleted edges are appeared in exported file again. After you close the knowledge box, you need to confirm the setting by enabling [Show Required Explicity]. So every time this issue comes up.

@yasu-sh
Copy link
Contributor Author

yasu-sh commented Oct 10, 2023

@fabiopuddu77 @jdramsey I can get the reproduction methods. I will edit the comment above.
image

@jdramsey jdramsey added the bug label Oct 12, 2023
@jdramsey
Copy link
Collaborator

OK let me try to address this. (It's the last of the Tetrad issues I want to address prior to releasing a new version next week, though there are a few py-terad/rpy-tetrad issues as well.) First let me see if I can reproduce if on my Mac. (I was having trouble with that before, but maybe with the additional instructions I could do it.)

@jdramsey
Copy link
Collaborator

OK I see what you mean now. Let me think about it after my meeting...

@jdramsey
Copy link
Collaborator

@yasu-sh, I figured out the issue. To fix another issue, we had to change the lists of required and forbidden edges from Sets to Lists, but after the change, when adding edges to one or the other of these lists, it was not checked whether they already belonged. If there were multiple copies, and an edge was removed, it would only remove the one (because it's a list), so another copy still remained.

I think it's fixed now. I'll double-check it and commit the fix in a few minutes.

jdramsey added a commit that referenced this issue Oct 12, 2023
@jdramsey
Fixed this issues:
640cd00 
#1678
@yasu-sh
Copy link
Contributor Author

yasu-sh commented Oct 12, 2023

@jdramsey It works! I appreciate your fixing this.
I checked Required Edges and Forbidden edges as well. Used commit
Both edges are OK. I am closing this.

@yasu-sh yasu-sh closed this as completed Oct 12, 2023
@jdramsey
Copy link
Collaborator

jdramsey commented Oct 12, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jdramsey @fabiopuddu77 @yasu-sh