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

Add FEFF schema #41

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add FEFF schema #41

wants to merge 7 commits into from

Conversation

msegal347
Copy link

Updated FEFF schemas.

Outstanding:

-Incorporating metadata in xmu.dat

-validate schema

Copy link
Contributor

@matthewcarbone matthewcarbone left a comment

Choose a reason for hiding this comment

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

Some minor changes but overall this is starting to look good.

I'm wondering if pydantic.Field should be "xas" instead of "FEFF", though, since these simulations are a type of XAS calculation.

@danielballan curious to hear your thoughts on this.

dataset: str
sample_id: str
#change to output log
output_script: str
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to output_log 👍

Also, needs a new key: xmu.dat-comments, which is also str.

@@ -145,3 +145,66 @@ class BatteryChargeMetadataInternal(pydantic.BaseModel):

class BatteryChargeMetadata(pydantic.BaseModel, extra=pydantic.Extra.allow):
charge: BatteryChargeMetadataInternal

class FEFFpotentials(pydantic.BaseModel, extra=pydantic.Extra.allow):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is too fine-grained. We can simply remove it for now. All of this is going into input_script.

#converted_potentials = str(FEFFpotentials)
#smiles = string

class FEFFcards(pydantic.BaseModel, extra=pydantic.Extra.allow):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as my previous comment:

This is too fine-grained. We can simply remove it for now. All of this is going into input_script.

measurement_type: MeasurementEnum = pydantic.Field("FEFF", const=True)
dataset: str
sample_id: str
input_script: str
Copy link
Contributor

Choose a reason for hiding this comment

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

input_script not input_scripts.

@matthewcarbone matthewcarbone changed the title update 2022OCT14 0921 Add FEFF schema Oct 17, 2022
@matthewcarbone
Copy link
Contributor

Actually @jmaruland I'd also like your thoughts on the above comments if you have the time. Want to be sure we're doing this correctly! Thank you!

@danielballan
Copy link
Contributor

Not sure. Seems like marking both xas and FEFF could be useful depending on the context. Can we think of sensible key names to place each of those values in?

@matthewcarbone
Copy link
Contributor

Sorry Dan what do you mean by this?

Can we think of sensible key names to place each of those values in?

My only comment is that $\mathrm{FEFF} \subset \mathrm{XAS}$, in case that matters.

@jmaruland
Copy link
Contributor

It feels that FEFF could contain additional required information that might not be necessary or required in XAS. Why not define FEFF as a child of XAS?
I am doing something similar with the Document schemas for this writer module that Dan and I use here to experiment with tiled. Is this similar to what are trying to describe here?
https://github.com/jmaruland/databroker/blob/master/databroker/experimental/schemas.py#L24-L28
https://github.com/jmaruland/databroker/blob/master/databroker/experimental/schemas.py#L31-L37
https://github.com/jmaruland/databroker/blob/master/databroker/experimental/schemas.py#L87-L88

@matthewcarbone
Copy link
Contributor

It feels that FEFF could contain additional required information that might not be necessary or required in XAS. Why not define FEFF as a child of XAS?

I think this is the way to go.

I am doing something similar with the Document schemas for this writer module that Dan and I use here to experiment with tiled. Is this similar to what are trying to describe here?

Yeah this is precisely what we're after, I think. I need to double check the XAS schema to still make sure it make sense, but at least in principle this is exactly how we should do it.

Copy link
Contributor

@matthewcarbone matthewcarbone left a comment

Choose a reason for hiding this comment

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

There are still a few issues here.

  • ingest needs to be a submodule of aimmdb. Remember when people import aimmdb they'll want access to all of the functions.
  • The notebook should be stripped of all output and should not be in ingest. I'd put it in a notebooks directory in root or something like this, but there should be no notebooks in the aimmdb module.
  • The tests that you wrote for this should go in aimmdb/_tests/ingest/feff.py
  • FEFFpotentials should be completely removed.

Copy link
Contributor

@matthewcarbone matthewcarbone left a comment

Choose a reason for hiding this comment

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

I think this is good but the notebook outputs need to be cleared.

"id": "1ff6333b-690f-4119-a2b2-bbe5c94c3112",
"metadata": {},
"outputs": [],
"outputs": [
Copy link
Contributor

Choose a reason for hiding this comment

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

All of the notebooks we store in GitHub should have their outputs stripped. This is because often outputs can inadvertently contain images or just lots of text, and this can be very space-intensive. There are a variety of ways to do this, including by using the command palette in Jupyter or by doing it from the command line, e.g. this.

@msegal347
Copy link
Author

msegal347 commented Oct 24, 2022

@danielballan @jmaruland @x94carbone

I'm still running into problems successfully writing the feff data and metadata to the client server, if you have a chance can you see what I'm doing wrong?

I've been trying to get the ingestion to work in the ingest_feff notebook. The dataframe and metadata seem to be outputting correctly, but when I use "client["uid"].write_dataframe" it's unsuccessful. I've tried write_sample as well, but I'm missing something here. Any help would be greatly appreciated!

Here's the output:

`timeout Traceback (most recent call last)
File c:\Users\msega\anaconda3\envs\aimm\lib\site-packages\httpcore_exceptions.py:8, in map_exceptions(map)
7 try:
----> 8 yield
9 except Exception as exc: # noqa: PIE786

File c:\Users\msega\anaconda3\envs\aimm\lib\site-packages\httpcore\backends\sync.py:26, in SyncStream.read(self, max_bytes, timeout)
25 self._sock.settimeout(timeout)
---> 26 return self._sock.recv(max_bytes)

timeout: timed out

During handling of the above exception, another exception occurred:

ReadTimeout Traceback (most recent call last)
File c:\Users\msega\anaconda3\envs\aimm\lib\site-packages\httpx_transports\default.py:60, in map_httpcore_exceptions()
59 try:
---> 60 yield
61 except Exception as exc: # noqa: PIE-786

File c:\Users\msega\anaconda3\envs\aimm\lib\site-packages\httpx_transports\default.py:218, in HTTPTransport.handle_request(self, request)
217 with map_httpcore_exceptions():
--> 218 resp = self._pool.handle_request(req)
220 assert isinstance(resp.stream, typing.Iterable)
...
74 raise
76 message = str(exc)
---> 77 raise mapped_exc(message) from exc

ReadTimeout: timed out`

@matthewcarbone
Copy link
Contributor

@msegal347 have you tried what Juan suggested and just try to inherit the existing XAS schema? I feel like if XAS works then inheriting it and adding a few properties might be easier than making our own new schema. Thoughts?

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.

4 participants