-
Notifications
You must be signed in to change notification settings - Fork 4
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
Hotfix/2.7.2 #152
Hotfix/2.7.2 #152
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will have to try, build and test the container with these changes; but here are a first set of comments.
Overall this is nice! thanks
import ROOT | ||
except ImportError: | ||
pass | ||
import ROOT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous try and except catching is required for the online documentation. Can you check this doesn't break anything?
If not, it might be better to leave this as it was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noah and I were trying to understand how this try/except clause makes sense – given that "ROOT" is used on the next line after the except clause. Can you please explain a bit more about why/how this is required or helpful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since ROOT is not installed on the Read the docs server via the setup.py script, when this PyBindRootProcessor file gets imported, it will fail.
This try and except is our way to handle the case of the ROOT package not being present, especially, this ROOT package cannot be installed.
The document is available here: https://morpho.readthedocs.io/en/latest/
Is this clearer this way?
If there is a better method to do this without killing readthedocs or if readthedocs is no longer maintained, we can for sure implement this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks. @nsoblath, do you know of any alternative approaches that might work, here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes me think that if you @taliaweiss have an account on RTD, I could add you as maintainer along with Noah and Ben (or whoever you think would be relevant)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ROOT import: I'm going to look into using this, and I'll test on RTD: https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#confval-autodoc_mock_imports
RTD account: @taliaweiss, if it's all right with you, I'll give you access to Morpho on RTD. I look at it so infrequently that I'll have to go remind myself how everything works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just made an account on RTD. @nsoblath, thank you for planning to give me access to morpho on RTD - that would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guiguem, I am trying to debug a failure of the IO test that occurs for the IOROOTProcessor. The error is included below.
It looks to me like the "x" and "y" data is written properly but being read wrong. The "myList" data might be getting written wrong, as well. I have been trying various changes to the pyROOT code, but I haven't gotten anywhere, yet. Do you have thoughts on how we might be able to fix this?
2021-07-08T16:26:57[DEBUG ] BaseProcessor(38) -> Creating processor <WriterROOT>
2021-07-08T16:26:57[DEBUG ] BaseProcessor(38) -> Creating processor <ReaderROOT>
2021-07-08T16:26:57[INFO ] BaseProcessor(52) -> Configure <WriterROOT>
2021-07-08T16:26:57[INFO ] BaseProcessor(52) -> Configure <ReaderROOT>
2021-07-08T16:26:57[INFO ] BaseProcessor(74) -> Run <WriterROOT>...
2021-07-08T16:26:57[DEBUG ] IOROOTProcessor(89) -> Saving data in myTest.root
2021-07-08T16:26:57[DEBUG ] IOROOTProcessor(96) -> Creating a file and tree
2021-07-08T16:26:57[DEBUG ] IOROOTProcessor(109) -> Defining tree properties
2021-07-08T16:26:57[DEBUG ] IOROOTProcessor(130) -> Updating number datapoints
2021-07-08T16:26:57[DEBUG ] IOROOTProcessor(201) -> None not supported; while use data to determine type
2021-07-08T16:26:57[DEBUG ] IOROOTProcessor(152) -> Creating branches
2021-07-08T16:26:57[DEBUG ] IOROOTProcessor(167) -> Adding data
2021-07-08T16:26:57[DEBUG ] IOROOTProcessor(181) -> File saved!
2021-07-08T16:26:57[INFO ] BaseProcessor(78) -> Done with <WriterROOT>
2021-07-08T16:26:57[INFO ] BaseProcessor(74) -> Run <ReaderROOT>...
2021-07-08T16:26:57[DEBUG ] IOROOTProcessor(51) -> Reading myTest.root
2021-07-08T16:26:57[WARNING ] IOROOTProcessor(62) -> An uproot related error was encountered. Switching to ROOT.
2021-07-08T16:26:57[INFO ] BaseProcessor(78) -> Done with <ReaderROOT>
2021-07-08T16:26:57[INFO ] IO_test(109) -> Data extracted = dict_keys(['x', 'y', 'myList'])
2021-07-08T16:26:57[INFO ] IO_test(111) -> x -> size = 6
2021-07-08T16:26:57[INFO ] IO_test(111) -> y -> size = 12
FAIL
======================================================================
FAIL: test_ROOTIO (__main__.IOTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "IO_test.py", line 112, in test_ROOTIO
self.assertEqual(len(data[key]), 6)
AssertionError: 12 != 6
----------------------------------------------------------------------
Ran 4 tests in 0.363s
FAILED (failures=1)
Misc testing
@@ -64,16 +64,17 @@ def Reader(self): | |||
import ROOT | |||
except ImportError: | |||
logger.warning("Failed importing ROOT") | |||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic below is completely different from what it was before... And indeed it doesn't seem to work.
I will push a new commit that is supposed to fix the logic.
The issue you spotted should be resolved with commit 22a35c5 which corrects 38db46f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't understand how printing myList
produced <cppyy.LowLevelView object at 0x7feb7a28dea0>
; I had seen that too.
Thanks so much for your new changes (commit 22a35c5
) – I ran the IO test and it worked! Awesome.
The remaining issue I see is that running the sampling test causes a segfault. I am looking into that further, now.
I see! |
Interesting – so for the values being read, do we want to check if each value is a cppyy.LowLevelView object (I'm not sure how to check this), and if it is, do I think this must happen to myList specifically because of some difficulty reading list (as opposed to floats or integers)? I.e., this is the result of |
…to hotfix/2.7.2
…to hotfix/2.7.2
Updating to use the new p8compute_dependencies image (v1.0.0).
Adapted the use of ROOT.TPyMultiGenFunction for ROOT v6.22+, which no longer has that class (see https://root.cern/doc/v622/release-notes.html#pyroot). Closes #151
Test: using both the p8c_dependencies v0.9.0 (earlier ROOT) and v1.0.0 (ROOT v6.22/6), open python and create a
morpho.PyFunctionObject
instance:A more thorough test should probably be run by a morpho expert.