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

Task 2 IFHM module pull request under other branch #20

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

hlee269
Copy link

@hlee269 hlee269 commented Jun 5, 2020

So, (1) I pushed the created branch (ifhm_ver01), which includes ifhm module, to Github and (2) reset the master branch to match exactly same as ymlasu repository.

Now, I am creating a pulling request using the created branch (ifhm_ver01). Please let me know if the way I did is wrong. I will correct it. Again, thanks for your help. I appreciate it!

@jmcfarland-swri
Copy link
Collaborator

Perfect, you're all set now.

I will take a look at the code. If any updates are needed, you can just add commits to your branch and then push them. The changes will automatically show up in this pull request.

Comment on lines +20 to +21
from jpype import *
from array import *
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, please avoid using import *. It makes it more difficult to maintain the code because you cannot tell where functions are coming from.

In this particular case, you may be able to remove these altogether. I'm not sure whether you are really using the array module (numpy provides much of the same functionality). Also, you probably do not need to call jpype directly in your module (I will add more detail about this separately).

Comment on lines +59 to +64
# Locate working directory to GNATS Client folder
os.chdir(GNATS_Client)

# Set classpath & start JVM (GNATS-Server)
classpath = "dist/gnats-client.jar:dist/gnats-shared.jar:dist/json.jar:dist/rmiio-2.1.2.jar:dist/commons-logging-1.2.jar"
startJVM(getDefaultJVMPath(), "-ea", "-Djava.class.path=%s" % classpath)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can all be replaced by a call to GnatsEnvironment.start_jvm, which comes from paraatm/io/gnats.py. In addition to doing this for you, it prevents multiple calls to jpype.startJVM, which would otherwise cause a crash (as written, your __init__ function cannot be called more than once). GnatsEnvironment will also handle automatically stopping the GNATS standalone, stopping the JVM, and restoring your original working directory.

# Define GNATS-Server environment (not standalone) & perform simulation
# Set PARA-ATM & GNATS folders
# PARA_ATM_Home = os.environ.get('PARA_ATM_Home')
GNATS_Home = os.environ.get("GNATS_Home")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The environment variable used by GNATS and para-atm is actually GNATS_HOME. If you make use of GnatsEnvironment, you might not need to use this environment variable directly.

Comment on lines +67 to +72
self.GNATS_SIMULATION_STATUS_READY = JPackage('com').osi.util.Constants.GNATS_SIMULATION_STATUS_READY
self.GNATS_SIMULATION_STATUS_START = JPackage('com').osi.util.Constants.GNATS_SIMULATION_STATUS_START
self.GNATS_SIMULATION_STATUS_PAUSE = JPackage('com').osi.util.Constants.GNATS_SIMULATION_STATUS_PAUSE
self.GNATS_SIMULATION_STATUS_RESUME = JPackage('com').osi.util.Constants.GNATS_SIMULATION_STATUS_RESUME
self.GNATS_SIMULATION_STATUS_STOP = JPackage('com').osi.util.Constants.GNATS_SIMULATION_STATUS_STOP
self.GNATS_SIMULATION_STATUS_ENDED = JPackage('com').osi.util.Constants.GNATS_SIMULATION_STATUS_ENDED
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can all be replaced by calls to GnatsEnvironment.get_nats_constant. For example, see gnats_gate_to_gate.py.

Comment on lines +74 to +75
GNATSClientFactory = JClass('GNATSClientFactory')
GnatsClient = GNATSClientFactory.getGNATSClient()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to use this instead of GNATS standalone? I am not sure about what the differences are, but the Python samples included with GNATS use the standalone instance. In para-atm, GnatsEnvironment.get_gnats_standalone is a helper function to get this for you. Note that part of the idea of all the helper functions in para-atm is to make it so that you don't need to use jpype directly.

Comment on lines +17 to +18
import os
os.chdir("/home/hector/Documents/para-atm-master")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to remove all references to your local filesystem. para-atm itself does not need to be run from any particular directory. If you are having trouble loading sample files that are stored inside para-atm, I can help with that.

from mpl_toolkits.mplot3d import Axes3D

# Set GNATS_Home and data directories
os.environ["GNATS_Home"] = "/home/hector/Documents/GNATS"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The environment variable for GNATS is GNATS_HOME.


# Set GNATS_Home and data directories
os.environ["GNATS_Home"] = "/home/hector/Documents/GNATS"
dir_data = "/home/hector/Documents/para-atm-master/paraatm/sample_data/ifhm"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to remove references to your local filesystem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants