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

Extension/lakeshore read curve file #1421

Merged
merged 17 commits into from
Jan 7, 2019

Conversation

sohailc
Copy link
Member

@sohailc sohailc commented Dec 20, 2018

Added the ability to parse and load curve files.

sochatoo added 3 commits December 19, 2018 17:05
2) bug fix in "upload_curve": should be index - 1 when selecting the right curve
Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

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

This looks quite nice! Could you cover the reading functions with some realistic tests, please?


def split_data_line(
line: str,
parsers: Union[type, List]=str
Copy link
Contributor

Choose a reason for hiding this comment

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

the List case seems to never been used, hence please remove the support for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok


Header1 Header2 Header3
data11 data12 data13
data21 data22 data23
Copy link
Contributor

Choose a reason for hiding this comment

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

please also document the format of the returned dictionary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok

the dictionary as expected by the 'upload_curve' method of the
driver

Note: This function modifies the input
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this decision made instead of letting this function return a new dictionary object?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah its cleaner to make and modify a copy

# Look up under the 'Data Format' entry to find what units we have
data_format = file_data['Data Format']
# This is a string in the form '4 (Log Ohms/Kelvin)'
data_format_int = int(data_format[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the total number of supported units? is it indeed 4 as valid_sensor_units suggests? regardless, i'd make this line a bit more robust via ... = int(data_format.split()[0]), is it fair to do so?

"""
_, filename = os.path.split(file_path)
if not filename.endswith(".330"):
raise ValueError("Only curve files with extension *.330 is supported")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise ValueError("Only curve files with extension *.330 is supported")
raise ValueError("Only curve files with extension *.330 are supported")

Upload a curve from a curve file. Note that we only support
curve files with extension *.330
"""
_, filename = os.path.split(file_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed? isn't filename.endswith(".330") == file_path.endswith(".330")?

@codecov
Copy link

codecov bot commented Dec 21, 2018

Codecov Report

Merging #1421 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1421   +/-   ##
=======================================
  Coverage   74.47%   74.47%           
=======================================
  Files          85       85           
  Lines        9960     9960           
=======================================
  Hits         7418     7418           
  Misses       2542     2542

@sohailc
Copy link
Member Author

sohailc commented Dec 21, 2018

@astafan8 Can you have another look

Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! Once you confirm that it has been tested on real files and real instrument, we can merge.

parser(i) for parser, i in zip(parsers, line_split)
]
def split_data_line(line: str, parser: type = str) -> List[str]:
return [parser(i) for i in line.split(" ") if i != ""]

def strip(strings: Iterable[str]) -> Tuple:
Copy link
Contributor

Choose a reason for hiding this comment

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

.. -> Tuple[str]?

@sohailc sohailc merged commit 7d7c316 into microsoft:master Jan 7, 2019
giulioungaretti pushed a commit that referenced this pull request Jan 7, 2019
Merge: f2ed8e4 81d4ec2
Author: sohail chatoor <schatoor@gmail.com>

    Merge pull request #1421 from sohailc/extension/lakeshore_read_curve_file
@sohailc sohailc deleted the extension/lakeshore_read_curve_file branch March 8, 2019 17:19
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.

None yet

2 participants