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

fixed dataset 58 and 58b, added time-series example #19

Closed
wants to merge 2 commits into from

Conversation

kb-
Copy link

@kb- kb- commented Oct 28, 2018

I fixed the following:
-Non ASCII copyright comment caused error (the file wouldn't run with the accents)
-Dataset 58, Record 7 fields 4,5,6 should be 3E13.5 according to specification
-58b: binary data was unreadable in other software

kb- and others added 2 commits October 28, 2018 02:26
fixed:
-Non ASCII copyright comment causing error
-Record 7 fields 4,5,6 should be 3E13.5 according to specification.
-58b: binary data unreadable in other software
@jankoslavic
Copy link
Contributor

Dear @kb- thank you for your effort. I am sorry, but I cannot accept a push request / patch like this.

  1. non-ascii problems: please provide a snippet test file
  2. Record 7: I have checked and tested this; it is ok. This should be corrected; please provide a separate pull request for this issue.
  3. The examples of use are shown in the Showcase file as noted in the readme; are you missing a 58b binary example?
  4. The code test are handled in the tests folder and there is also a 58b test.
  5. The changes with regards to the 58b in line 969, the changes are from
if bo == 1:
                    [fh.write(struct.pack('<d', datai)) for datai in data]
                else:
                    [fh.write(struct.pack('>d', datai)) for datai in data]

to:

fh.write(bytearray(data))   

Please give more details why the current implementation does not work for you and provide a snippet test file.

@kb-
Copy link
Author

kb- commented Oct 28, 2018

  1. import pyuff gives me the following error with Python 2.7. ok with 3.6

File "pyuff.py", line 1
# Copyright (C) 2014-2017 Primož Čermelj, Matjaž Mršnik, Miha Pirnat, Janko Slavič, Blaž Starc (in alphabetic order)
^
SyntaxError: Non-ASCII character '\xc5' in file pyuff.py on line 1, but no encoding declared; see http://python.org/dev/peps/pep-0263/ for details

  1. I was missing both a binary and an example to write time series.
  2. 58b produced by your code fails to be read properly in commercial software (nCode). I can try different softwares if necessary. The proposed modification is working for np.single datatype only.
    test58b_double_original.uff : your code, passed np.double datatype
    image

test58b_single_original.uff : your code, passed np.singledatatype
image

test58b_double_modified.uff : modified code, np.double datatype
image

test58b_single_modified.uff : OK, modified code, passed np.singledatatype
image

@jankoslavic
Copy link
Contributor

Thanks for the feedback.

  1. is related to the closed issue: DEV: add py2 support #1

  2. If I understand correctly, the provided examples are written with pyuff and read inside nCode. Is it possible to get nCode exported 58b? I could have this file to prepare reading/writing nCode round test.

@kb-
Copy link
Author

kb- commented Oct 29, 2018

  1. Ok, it would be nice to make it obvious either by writing it in the module description and README or by throwing an error message. Python 2 or 3 compatibility is often a frustration, especially when we don't know why a module isn't working. Your module seems to work fine on Python 2 after removing the accents tho.

  2. nCode can't generate this type of file unfortunately, only import. I'll get back to you when I can find this type of files from another source. Probably next week.

@jankoslavic
Copy link
Contributor

  1. if the non ascii characters in the comments are a problem, I have added file encoding to the files. Is it working for you now?
  2. this would be great!

@jankoslavic
Copy link
Contributor

PS: the last commit includes 13.5e formating (in all fields, not only the ones you pointed out; I do not remember why we were using 13.4e; I hope it will not break other people's code - all test are working fine) Can you please double check your files on this?

@jankoslavic jankoslavic closed this Oct 3, 2019
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