cmu-phil / tetrad

Repository for the Tetrad Project, www.phil.cmu.edu/tetrad.
GNU General Public License v2.0
404 stars 110 forks source link

"Processing" button is always indicated at display 1 and not movable at windows machine #1674

Closed yasu-sh closed 8 months ago

yasu-sh commented 1 year ago

@jdramsey congraturations on the new release. I guess many bug fixes done. This is not important. a small note when @jdramsey have a chance to fix.

When I use TETRAD in display 2, the process window indicates in display 1 without title bar. So I cannot move the button, which is like hovering in the air. I wish the button have title bar, or indicates in current display. If considering on the multi-platform, the button with title bar may makes sense.

image

OS: Windows 10 22H2 Memory: 32GB

java '-Duser.language=en' --version openjdk 11.0.18 2023-01-17 OpenJDK Runtime Environment Temurin-11.0.18+10 (build 11.0.18+10) OpenJDK 64-Bit Server VM Temurin-11.0.18+10 (build 11.0.18+10, mixed mode)

jdramsey commented 1 year ago

Interesting. I need to think of a way to test that. I could try it in my office with an external monitor.

Question. One thing I decided to do is make a more modern GUI app using JavaFX. To test the idea, I made a project in which I display a very large DAG, something that's difficult to do in the current interface--it often hangs the entire interface if you try to do that. So there's some motivation for updating that particular component.

The problem is, no end-user has ever asked for this to be done, and there are some workarounds. Also, we wouldn't be able to support Java 1.8 with JavaFX. So probably I should drop the idea.

But what do you think?

yasu-sh commented 1 year ago

@jdramsey Thanks for checking the symptom. I am afraid that I was out of the office for a week.

Answer: It is reasonable idea for you to think more large DAG manupilation. I guess JavaFX enableds UI to have better performance or flexible zoom in-out. (but not necessary).

I doubt if Android UI SDK has many features. And it is not usable for normal Mac/PCs. Java 8 is not mandatory now since Java 17 is available and better capability. (EOL is similar) JavaFX is reasonalbe choice now. Note that this is my personal view.

jdramsey commented 1 year ago

@yasu-sh I did go ahead and make a "first draft" of a JavaFX app for Tetrad. The repository is here:

https://github.com/cmu-phil/tetrad-fx

What do you think? Should I continue with the project or drop it? (Don't worry, I'm asking several people so that you can give your honest opinion.)

jdramsey commented 1 year ago

@yasu-sh Hmm... this I don't know how to fix. For one thing, I don't have a Windows machine with multiple monitors. I could try it using an external monitor for my mac laptop though.

yasu-sh commented 1 year ago

No problem so far. The Program/GUI is functional. I just exchanged primary monitor with secondary one when I had annoying situation.

@yasu-sh Hmm... this I don't know how to fix. For one thing, I don't have a Windows machine with multiple monitors. I could try it using an external monitor for my mac laptop though.

yasu-sh commented 1 year ago

@jdramsey Thank you for your asking. I like this workflow-oriented design. When I started using tetrad, I have easily got lost the next step. Pipelines enabled me to find basic usage. I had difficulties to try different hyperparameters. When I have many conditions, I needed to make notes and lost my condition. So I think the new design works well for my case.

For causal discovery use cases, the orignal tetrad would works. Charts / correlation matrix / visualization is also necessary for making good result from causal inference. My concern is how to deal with causal inference/SEM parameters' UI. It would be nicer, but it seems to become tough tasks.

Permutation game is interesting and it is good for learners to understand algorithms. Constraint-based algorithm's visualizaton is useful for me when I explain the principle to colleagues.(personal usage)

As an answer, difficult. Let me have some times for consideration.

@yasu-sh I did go ahead and make a "first draft" of a JavaFX app for Tetrad. The repository is here:

https://github.com/cmu-phil/tetrad-fx

What do you think? Should I continue with the project or drop it? (Don't worry, I'm asking several people so that you can give your honest opinion.)

jdramsey commented 1 year ago

Thank you so much for your comments! There is no need to make the original Tetrad unavailable, even if we move forward with Tetrad-FX. They would be just two interfaces to the same underlying library, as are py-tetrad, rpy-tetrad, and causal-cmd. One could use whichever one finds more helpful.

That said, I'm so happy you find the workflow-oriented design helpful! If we continue, I will add all of the things that you mentioned. And I am so happy you like the permutation game! I will add more games then! :-D

yasu-sh commented 1 year ago

It's great and It is important that existing library works well. Recently I understood why your recommended py-tetrad and it can pull out all functionalities from jar file.

Regarding the workflow style, the unpredictable try-and-error preprocess would be challenging. Tha's why many analysis tool adopts flowchart style like dataiku/KNIME/Orange.

For me, the original tetrad style is okay and it works, even still I did not understand how to use apporipately in SEM. As for international support, tetrad-fx would make good progress. I am curios on it.


By the way, I recently found an OSS - RATH. https://github.com/Kanaries/Rath Demonstration site: https://rath.kanaries.net/

You can try causal-service as alpha version. Algorithm name seems to be invalid(XLearner means probabily FCI),

This works by copying codes from causal-learn repos. (They should follow MIT License(causal-learn) whereas they use AGPLv3) https://github.com/Kanaries/Rath/tree/master/services/causal-service/algorithms

But UI and simulation/estimation/visualization looks interesting and some insight would come up to us. image

jdramsey commented 11 months ago

@yasu-sh I was thinking about the issue you raised of getting the Processing button to follow the selected monitor. I did some research, and that would seem to require using platform-specific code, which we've been avoiding. I have another idea, which I'll think about some. I've noticed that a lot of algorithms that let you run processes have buttons for Run/Stop in the main frame itself rather than popping up a dialog for this purpose, like IntelliJ. I need to think about how to do that; it may be a better idea. I wouldn't need to pop up a dialog for that, though I'd have to handle the processes differently, so it could be a fair amount of work on my end.

yasu-sh commented 10 months ago

@jdramsey Do you mean this one in IntelliJ? image or this one? image

The former one is the behavior I expected. The latter one depends the implementation, background process monitoring. I did not notice the behavior is platform-specific. If you think this is difficult to solve, I just do not care and I hope you add this issue to "wish-list" in TETRAD.

jdramsey commented 8 months ago

Huh, on the screen-tracking issue, ChatGPT now gives this response, which may be doable:

import javax.swing.; import java.awt.; import java.awt.event.ComponentAdapter; import java.awt.event.ComponentEvent;

public class Main {

public static void main(String[] args) {
    JFrame frame = new JFrame("Main Application");
    JDialog dialog = new JDialog(frame, "Dialog");

    frame.setSize(400, 300);
    frame.setLocationRelativeTo(null); // Center the frame
    frame.setVisible(true);

    dialog.setSize(200, 150);
    dialog.setLocationRelativeTo(frame); // Position dialog relative to frame
    dialog.setVisible(true);

    frame.addComponentListener(new ComponentAdapter() {
        @Override
        public void componentMoved(ComponentEvent e) {
            moveDialogToSameScreen(frame, dialog);
        }
    });
}

private static void moveDialogToSameScreen(JFrame frame, JDialog dialog) {
    Point frameLocation = frame.getLocation();
    GraphicsEnvironment ge = GraphicsEnvironment.getLocalGraphicsEnvironment();
    GraphicsDevice[] screens = ge.getScreenDevices();

    for (GraphicsDevice screen : screens) {
        Rectangle bounds = screen.getDefaultConfiguration().getBounds();
        if (bounds.contains(frameLocation)) {
            Point dialogLocation = dialog.getLocation();
            dialogLocation.translate(bounds.x - frameLocation.x, bounds.y - frameLocation.y);
            dialog.setLocation(dialogLocation);
            break;
        }
    }
}

}

jdramsey commented 8 months ago

I added this code to my branch, but I don't have a way right now to test it to see if it works. Next time I have an external monitor hooked up I'll test it.

jdramsey commented 8 months ago

Actually, ChatGPT came up with an even better idea--I added a component listener that always centers the watch dialog on the frame no matter where the frame goes. It works if I move the frame on my laptop screen, and it is kind of nice. I still need to test it on multiple monitors.

yasu-sh commented 8 months ago

@jdramsey Thanks a lot for your trials! I've just pulled the development branch and tried to build. It looks working on Java 17(I used Java 11 for build) on Win10 machine.

I found some bugs need to be fixed. ex. when the window(not button) moves, IntelliJ IDEA's console indicates exeptions continously.

Exception in thread "AWT-EventQueue-0" java.lang.NullPointerException: Cannot invoke "javax.swing.JDialog.getLocation()" because "dialog" is null

But I believe you can fix this issue.

jdramsey commented 8 months ago

Could you pull from joe-work-2023-11-15 instead? That's where I did the fix... the pull request hasn't been merged yet... sorry for not telling you...

jdramsey commented 8 months ago

You know, I would add you as a reviewer for Tetrad, but we'd need to pay for a seat for you on GitHub, which is about $50. It might be worth it, though. Let me see if anyone can be dropped at this point.

jdramsey commented 8 months ago

Actually, there were several people who didn't need that kind of access to the repository anymore, so I just removed them all and added you. If you accept the invite, I can add you as a reviewer for the pull request:

https://github.com/cmu-phil/tetrad/pull/1716

jdramsey commented 8 months ago

These are the forthcoming fixes, in case you want to check anything else out:

https://github.com/cmu-phil/tetrad/wiki/Forthcoming-fixes

yasu-sh commented 8 months ago

Could you pull from joe-work-2023-11-15 instead? That's where I did the fix... the pull request hasn't been merged yet... sorry for not telling you...

No problem! I should have inquired on it. I used the branch this time. No error comes up during my moving window now.

Last 2 things to note:

1. In console, Tetrad's startup indicates the exeption as below. It could be related to Windows. So far the functionality is okay.

"Java\jdk-17\bin\java.exe" (omit) Exception in thread "AWT-EventQueue-0" java.lang.UnsupportedOperationException: The APP_QUIT_HANDLER action is not supported on the current platform! at java.desktop/java.awt.Desktop.checkActionSupport(Desktop.java:381) at java.desktop/java.awt.Desktop.setQuitHandler(Desktop.java:848) at edu.cmu.tetradapp.Tetrad.launchFrame(Tetrad.java:195) at edu.cmu.tetradapp.Tetrad.lambda$main$0(Tetrad.java:101) at java.desktop/java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:318) at java.desktop/java.awt.EventQueue.dispatchEventImpl(EventQueue.java:771) at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:722) at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:716) at java.base/java.security.AccessController.doPrivileged(AccessController.java:399) at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:86) at java.desktop/java.awt.EventQueue.dispatchEvent(EventQueue.java:741) at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:203) at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:124) at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:113) at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:109) at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101) at java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:90)

2. simulation box(data creation) makes the dialog only for main display(this time 2nd display).

image

Other boxes like Search Box or Compare Box makes the dialog at the same display. This is small matter.

yasu-sh commented 8 months ago

Actually, there were several people who didn't need that kind of access to the repository anymore, so I just removed them all and added you. If you accept the invite, I can add you as a reviewer for the pull request:

@jdramsey Thanks for your proposal and invitation! Once I accepted it, then I notice you need to make some payment for outside collaboratior on Github. This is the first time for me to act as collaborator at other organizations. (I've used GitLab in our office)

Until today I have used the forked repository for making pull-request. I'll check what I can do for TETRAD.

cg09 commented 8 months ago

What payment is needed to whom?

Clark

On Thu, Jan 25, 2024 at 9:40 PM Yasuhiro Shimodaira < @.***> wrote:

Actually, there were several people who didn't need that kind of access to the repository anymore, so I just removed them all and added you. If you accept the invite, I can add you as a reviewer for the pull request:

Thanks for your proposal and invitation! Once I accepted it, then I notice you need to some payment. This is the first time for me to act in collaborator at other organizations. (I've used GitLab in our office)

Until today I have used the forked repository for making pull-request. I'll check what I can do for TETRAD.

— Reply to this email directly, view it on GitHub https://github.com/cmu-phil/tetrad/issues/1674#issuecomment-1911319811, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD4Y3OIZGMVRKJAQSMTHYWLYQMJRLAVCNFSM6AAAAAA3GUNX7GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJRGMYTSOBRGE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

jdramsey commented 8 months ago

@cg09 Hopefully, this is a payment I can cover.

jdramsey commented 8 months ago

@yasu-sh Oh that is a good point; I only made that change for the Processing... dialog. Let me think if I can make it for all dialogs--it may be a bit of work.

jdramsey commented 8 months ago

I think I can put a try-catch block around the quit handler code to prevent that error message.

Also, so far you're the only vote in favor of Tetrad-FX! But if there is a vote, I will spend some time on it! :-)

jdramsey commented 8 months ago

@cg09 Looks like there are no worries about the charge.

yasu-sh commented 8 months ago

What payment is needed to whom? Clark @cg09 Looks like there are no worries about the charge.

If you have some concerns, let me know. There must be no obligation for cancellation.

I think I can put a try-catch block around the quit handler code to prevent that error message. Also, so far you're the only vote in favor of Tetrad-FX! But if there is a vote, I will spend some time on it! :-)

@jdramsey Thanks.

yasu-sh commented 8 months ago

@jdramsey I have checked the issues solved. I appreciate a lot.

  1. Fixed: Process button indication is done on the same display(1st post)
  2. Fixed: New dataset dialog is indicated in the same window
  3. Fixed: Exeptions when moving window

Confirmed at: https://github.com/cmu-phil/tetrad/commit/76318f0c51e660feb79159c60e80f065992024f4

jdramsey commented 8 months ago

Awesome! Thanks!