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

Hotfix/2.7.2 #152

Merged
merged 42 commits into from
Jul 20, 2021
Merged

Hotfix/2.7.2 #152

merged 42 commits into from
Jul 20, 2021

Conversation

nsoblath
Copy link
Contributor

@nsoblath nsoblath commented Jul 7, 2021

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:

>>> test = morpho.PyFunctionObject(5)
2021-07-07T18:20:32[INFO    ] PyBindRooFitProcessor(33) -> Created PyFunctionObject

A more thorough test should probably be run by a morpho expert.

Copy link
Contributor

@guiguem guiguem left a 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
Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor

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...

Copy link
Contributor

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?

Copy link
Contributor

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)

Copy link
Contributor Author

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.

Copy link
Contributor

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.

morpho/processors/sampling/PyBindRooFitProcessor.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
tests/test.sh Show resolved Hide resolved
Copy link
Contributor

@taliaweiss taliaweiss left a 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
Copy link
Contributor

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.

@guiguem
Copy link
Contributor

guiguem commented Jul 8, 2021

The issue you spotted should be resolved with commit 22a35c5 which corrects 38db46f
I am not sure which this was so deeply changed in the first place.
In particular, this is unclear to me how lists in root files are now handled; as far as I can tell, doing a print of myList produces <cppyy.LowLevelView object at 0x7feb7a28dea0> which is not something I know how to interpret...

Copy link
Contributor

@taliaweiss taliaweiss left a 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.

@guiguem
Copy link
Contributor

guiguem commented Jul 8, 2021

I see!
I guess then we will need to find a way to interpret this "myList".
Maybe something like what is done here: https://cppyy.readthedocs.io/en/latest/lowlevel.html#c-c-casts

@taliaweiss
Copy link
Contributor

taliaweiss commented Jul 8, 2021

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 cppyy.bind_object and then use fInt or similar?

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 getattr(tree, varName) in the case of lists?

@taliaweiss
Copy link
Contributor

I added a conditional that makes the IOROOTProcessor read myList properly: commit 3dc7d77. (My commit 2a467f2 also worked for this purpose, but it used a try/except statement, which l think is not the right approach.)

@taliaweiss taliaweiss merged commit 1d5b508 into develop Jul 20, 2021
@taliaweiss taliaweiss deleted the hotfix/2.7.2 branch July 20, 2021 20:27
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.

AttributeError: Failed to get attribute TPyMultiGenFunction from ROOT for ROOT 6.22
3 participants