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 multi_pass_weld-0.1.0.yaml #667

Merged
merged 11 commits into from
Feb 17, 2022

Conversation

CagtayFabry
Copy link
Member

Changes

Describe changes in this PR

Related Issues

Closes # (add issue numbers)

Checks

  • updated CHANGELOG.rst
  • updated tests
  • updated doc/
  • update example/tutorial notebooks
  • update manifest file

@CagtayFabry CagtayFabry added the ASDF everything ASDF related (python + schemas) label Dec 7, 2021
@CagtayFabry CagtayFabry added this to the 0.6.0 milestone Dec 7, 2021
Comment on lines +53 to +55
TCP:
description: |
Transformation describing the welding TCP movement in relation to the groove coordinates.
Copy link
Member Author

@CagtayFabry CagtayFabry Dec 7, 2021

Choose a reason for hiding this comment

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

I have taken this from the single_pass_weld schema but I think in general it would be more generic to describe the contact tip position and orientation (with an optional target stickout length)

@github-actions
Copy link

github-actions bot commented Dec 7, 2021

Unit Test Results

       1 files  ±0         1 suites  ±0   1m 34s ⏱️ -14s
2 156 tests ±0  2 156 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit e419baa. ± Comparison against base commit 23fe9c9.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Dec 7, 2021

Codecov Report

Merging #667 (e419baa) into master (23fe9c9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #667   +/-   ##
=======================================
  Coverage   96.05%   96.05%           
=======================================
  Files          92       92           
  Lines        6393     6393           
=======================================
  Hits         6141     6141           
  Misses        252      252           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23fe9c9...e419baa. Read the comment docs.

@CagtayFabry CagtayFabry mentioned this pull request Dec 9, 2021
5 tasks
@vhirtham
Copy link
Collaborator

vhirtham commented Dec 17, 2021

As already mentioned yesterday, the references in the definitions do not work. So I tested it with the following script and schema:

import asdf

tree = dict(
    # a=dict(b=dict(c="This")),
    # a2=[dict(c="is")],
    # a3=[dict(b=[dict(c="strange")])],
    aa=dict(b=dict(c="This")),
    aa2=[[dict(c="is")]],
    aa3=[dict(b=[dict(c="strange")])],
)

with asdf.AsdfFile(tree, custom_schema="test_schema.yaml") as ff:
    ff.write_to("test.asdf")
%YAML 1.1
---
$schema: "http://stsci.edu/schemas/yaml-schema/draft-01"

definitions:
  C:
    type: object
    properties:
      c:
    required: [c]

  B:
    type: object
    properties:
      b:
        $ref: "#/definitions/C"
    required: [b]

  B2:
    type: array
    items:
        $ref: "#/definitions/C"


  B3:
    type: array
    items:
      type: object
      properties:
        b:
          type: array
          items:
            $ref: "#/definitions/C"
      required: [ b ]

type: object
properties:
    a:
      $ref: "#/definitions/B"
    a2:
      $ref: "#/definitions/B2"
    a3:
      $ref: "#/definitions/B3"

The aa members of the dictionary are just copies of the a members that are disabled via comments. In contrast to the a members, the aas don't get validated by the schema and everything passes. As soon as you enable one of the other members, the following error gets raised:

jsonschema.exceptions.RefResolutionError: Unresolvable JSON pointer: 'definitions/C'

Seems like nested $ref calls are not supported by either JSON schema or the yaml-ized version asdf is using. The question is how we want to solve this. We can simply introduce tagged objects to circumvent the problem. What we can also do is just copy-pasting everything into the first referenced object but this results in a very unpleasant definition. I did it to test serializing our experimental data against the schema. So it works (why shouldn't it?), but it is really ugly.

Another thing I wanted to mention: The way nested arrays are stored is also not very nice:

my_array
- - - v: a
    - v: b
  - - v: c
  - - v: d
- - - v: e
    - v: f

It gets messy quickly and it is hard to see where a new array is starting at which nesting level, especially with larger objects. This should also be resolved by tagged objects since there will be at least a separating tag (IIRC) and probably another property name to store the next array.

@CagtayFabry
Copy link
Member Author

thank you for the report @vhirtham :)

One possible issue I could see with the nested references is that it might break when using a "relative" reference (that is not resolvable within a nesting)

So we might try to use absolute schema references here like
$ref: "asdf://weldx.bam.de/weldx/schemas/datamodels/multi_pass_weld-0.1.0#/definitions/weld_bead"

or maybe it works on the file level with $ref: "./multi_pass_weld-0.1.0#/definitions/weld_bead"

if the absolute reference doesn't work we should probably check in with the asdf devs

@vhirtham
Copy link
Collaborator

Okay, got the $ref stuff sorted out. The problem is, that the schema has no $id. If one adds

id: "asdf://weldx.bam.de/weldx/schemas/datamodels/test_schema-0.1.0"

and actually puts the schema in the corresponding directory, the relative references work without further problems. So I have to create a dev kernel on the server and use a working branch to further develop the schema and apply it to the experimental data using the already existing jupyter notebook. However, I think I will draft it locally with dummy data before trying it on the real data. Should save some time by avoiding huge data chunks.

@CagtayFabry
Copy link
Member Author

Okay, got the $ref stuff sorted out. The problem is, that the schema has no $id. If one adds

id: "asdf://weldx.bam.de/weldx/schemas/datamodels/test_schema-0.1.0"

and actually puts the schema in the corresponding directory, the relative references work without further problems. So I have to create a dev kernel on the server and use a working branch to further develop the schema and apply it to the experimental data using the already existing jupyter notebook. However, I think I will draft it locally with dummy data before trying it on the real data. Should save some time by avoiding huge data chunks.

can you please update the schema on this branch (it is already tagged with an id) to a working state? @vhirtham

for a simple dev kernel on JupyterHub, this should work:

mamba create -n weldx_test python=3.9
mamba activate weldx
mamba install weldx --only-deps

then register the kernel in this environment, clone this repository/branch and local pip install

@vhirtham
Copy link
Collaborator

The schema works with the experimental data, but we should still discuss this:

Another thing I wanted to mention: The way nested arrays are stored is also not very nice:

my_array

- - - v: a
    - v: b
  - - v: c
  - - v: d
- - - v: e
    - v: f

@CagtayFabry
Copy link
Member Author

The schema works with the experimental data, but we should still discuss this:

Another thing I wanted to mention: The way nested arrays are stored is also not very nice:
my_array

- - - v: a
    - v: b
  - - v: c
  - - v: d
- - - v: e
    - v: f

It's not pretty but I still think this is the most 'natural' way to define the schema

how does it look in our WeldxFile.info or WeldxFile.show_asdf_header outputs?

@vhirtham vhirtham marked this pull request as ready for review February 16, 2022 15:37
@vhirtham vhirtham merged commit 8a2ccec into BAMWelDX:master Feb 17, 2022
@CagtayFabry CagtayFabry deleted the multi_pass_weld_schema branch February 17, 2022 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASDF everything ASDF related (python + schemas)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants